Skip to content

Commit

Permalink
avoid multiprocessing when we update <50 releases
Browse files Browse the repository at this point in the history
  • Loading branch information
azuline committed Oct 29, 2023
1 parent 495fb99 commit d34dcde
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 6 deletions.
16 changes: 16 additions & 0 deletions rose/cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,8 @@ def update_cache_for_releases(
# Leave as None to update all releases.
release_dirs: list[Path] | None = None,
force: bool = False,
# For testing.
force_multiprocessing: bool = False,
) -> None:
"""
Update the read cache to match the data for any passed-in releases. If a directory lacks a
Expand Down Expand Up @@ -296,9 +298,23 @@ def update_cache_for_releases(
and d.name != "!playlists"
and d.name not in c.ignore_release_directories
]
if not release_dirs:
logger.info("No-Op: No whitelisted releases passed into update_cache_for_releases")
return
logger.info(f"Refreshing the read cache for {len(release_dirs)} releases")
logger.debug(f"Refreshing cached data for {', '.join([r.name for r in release_dirs])}")

# If the number of releases changed is less than 50; do not bother with all that multiprocessing
# gunk: instead, directly call the executor.
#
# This has an added benefit of not spawning processes from the virtual filesystem and watchdog
# processes, as those processes always update the cache for one release at a time and are
# multithreaded. Starting other processes from threads is bad!
if not force_multiprocessing and len(release_dirs) < 50:
known_virtual_dirnames_hi: dict[str, bool] = {}
_update_cache_for_releases_executor(c, release_dirs, force, known_virtual_dirnames_hi)
return

# Batch size defaults to equal split across all processes. However, if the number of directories
# is small, we shrink the # of processes to save on overhead.
num_proc = c.max_proc
Expand Down
12 changes: 12 additions & 0 deletions rose/cache_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,18 @@ def test_update_cache_all(config: Config) -> None:
assert cursor.fetchone()[0] == 4


def test_update_cache_multiprocessing(config: Config) -> None:
"""Test that the update all function works."""
shutil.copytree(TEST_RELEASE_1, config.music_source_dir / TEST_RELEASE_1.name)
shutil.copytree(TEST_RELEASE_2, config.music_source_dir / TEST_RELEASE_2.name)
update_cache_for_releases(config, force_multiprocessing=True)
with connect(config) as conn:
cursor = conn.execute("SELECT COUNT(*) FROM releases")
assert cursor.fetchone()[0] == 2
cursor = conn.execute("SELECT COUNT(*) FROM tracks")
assert cursor.fetchone()[0] == 4


def test_update_cache_releases(config: Config) -> None:
release_dir = config.music_source_dir / TEST_RELEASE_1.name
shutil.copytree(TEST_RELEASE_1, release_dir)
Expand Down
3 changes: 0 additions & 3 deletions rose/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
"""

import logging
import multiprocessing
import os
import signal
from dataclasses import dataclass
Expand Down Expand Up @@ -95,7 +94,6 @@ def update(ctx: Context, force: bool) -> None:
# fmt: on
def watch(ctx: Context, foreground: bool) -> None:
"""Start a watchdog that will auto-refresh the cache on changes in music_source_dir."""
multiprocessing.set_start_method("spawn")
if not foreground:
daemonize(pid_path=ctx.config.watchdog_pid_path)

Expand Down Expand Up @@ -131,7 +129,6 @@ def fs() -> None:
# fmt: on
def mount(ctx: Context, foreground: bool) -> None:
"""Mount the virtual library."""
multiprocessing.set_start_method("spawn")
if not foreground:
daemonize()

Expand Down
3 changes: 0 additions & 3 deletions rose/cli_test.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import multiprocessing
import os
from typing import Any

Expand Down Expand Up @@ -35,8 +34,6 @@ def mock_exit(x: int) -> None:
raise SystemExit(x)

monkeypatch.setattr(os, "_exit", mock_exit)
# set_start_method borks a bit in tests; it's not important in this test anyways.
monkeypatch.setattr(multiprocessing, "set_start_method", lambda _: None)

ctx = Context(config=config)
runner = CliRunner()
Expand Down

0 comments on commit d34dcde

Please sign in to comment.