From e18906b4fc5b5753e85c654678783e0a06aa3e57 Mon Sep 17 00:00:00 2001 From: Thomas Barlow Date: Tue, 29 Oct 2024 18:51:17 +0000 Subject: [PATCH 1/6] Zillion: Remove generate_output and fill_slot_data thread synchronization `finalize_item_locations()` does not mutate `self` or any attribute of `self`, so fill_slot_data does not need to wait for `finalize_item_locations()` to be run. Previously, before 113c54f9bea7, synchronization was necessary because `finalize_item_locations()` would set location data into `self.zz_system.patcher`, which `fill_slot_data` would then read from. This also means that Zillion can be re-enabled for the test.general.test_implemented.test_slot_data test. --- test/general/test_implemented.py | 2 +- worlds/zillion/__init__.py | 11 +---------- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/test/general/test_implemented.py b/test/general/test_implemented.py index e76d539451ea..74ae2459faee 100644 --- a/test/general/test_implemented.py +++ b/test/general/test_implemented.py @@ -39,7 +39,7 @@ def test_slot_data(self): """Tests that if a world creates slot data, it's json serializable.""" for game_name, world_type in AutoWorldRegister.world_types.items(): # has an await for generate_output which isn't being called - if game_name in {"Ocarina of Time", "Zillion"}: + if game_name in {"Ocarina of Time"}: continue multiworld = setup_solo_multiworld(world_type) with self.subTest(game=game_name, seed=multiworld.seed): diff --git a/worlds/zillion/__init__.py b/worlds/zillion/__init__.py index d5e86bb33292..35817c6b02ad 100644 --- a/worlds/zillion/__init__.py +++ b/worlds/zillion/__init__.py @@ -118,8 +118,6 @@ def flush(self) -> None: """ my_locations: List[ZillionLocation] = [] """ This is kind of a cache to avoid iterating through all the multiworld locations in logic. """ - slot_data_ready: threading.Event - """ This event is set in `generate_output` when the data is ready for `fill_slot_data` """ logic_cache: Union[ZillionLogicCache, None] = None def __init__(self, world: MultiWorld, player: int): @@ -127,7 +125,6 @@ def __init__(self, world: MultiWorld, player: int): self.logger = logging.getLogger("Zillion") self.lsi = ZillionWorld.LogStreamInterface(self.logger) self.zz_system = System() - self.slot_data_ready = threading.Event() def _make_item_maps(self, start_char: Chars) -> None: _id_to_name, _id_to_zz_id, id_to_zz_item = make_id_to_others(start_char) @@ -365,12 +362,7 @@ def finalize_item_locations(self) -> GenData: def generate_output(self, output_directory: str) -> None: """This method gets called from a threadpool, do not use multiworld.random here. If you need any last-second randomization, use self.random instead.""" - try: - gen_data = self.finalize_item_locations() - except BaseException: - raise - finally: - self.slot_data_ready.set() + gen_data = self.finalize_item_locations() out_file_base = self.multiworld.get_out_file_name_base(self.player) @@ -393,7 +385,6 @@ def fill_slot_data(self) -> ZillionSlotInfo: # json of WebHostLib.models.Slot # TODO: tell client which canisters are keywords # so it can open and get those when restoring doors - self.slot_data_ready.wait() assert self.zz_system.randomizer, "didn't get randomizer from generate_early" game = self.zz_system.get_game() return get_slot_info(game.regions, game.char_order[0], game.loc_name_2_pretty) From 73f30deb903f1fe01eb8f77bf6cadd4340f45707 Mon Sep 17 00:00:00 2001 From: Thomas Barlow Date: Tue, 29 Oct 2024 23:07:50 +0000 Subject: [PATCH 2/6] Also remove the threading import now that it is unused --- worlds/zillion/__init__.py | 1 - 1 file changed, 1 deletion(-) diff --git a/worlds/zillion/__init__.py b/worlds/zillion/__init__.py index 35817c6b02ad..d03568fe9eaf 100644 --- a/worlds/zillion/__init__.py +++ b/worlds/zillion/__init__.py @@ -2,7 +2,6 @@ from contextlib import redirect_stdout import functools import settings -import threading import typing from typing import Any, Dict, List, Set, Tuple, Optional, Union import os From d4afc7888c7e2e56f4384f8e31a741bde1fd4f8e Mon Sep 17 00:00:00 2001 From: Thomas Barlow Date: Wed, 30 Oct 2024 16:39:55 +0000 Subject: [PATCH 3/6] Revert "Also remove the threading import now that it is unused" This reverts commit 73f30deb903f1fe01eb8f77bf6cadd4340f45707. --- worlds/zillion/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/worlds/zillion/__init__.py b/worlds/zillion/__init__.py index d03568fe9eaf..35817c6b02ad 100644 --- a/worlds/zillion/__init__.py +++ b/worlds/zillion/__init__.py @@ -2,6 +2,7 @@ from contextlib import redirect_stdout import functools import settings +import threading import typing from typing import Any, Dict, List, Set, Tuple, Optional, Union import os From 631a3e222d6cc5e50845e34b4fa6fe7964dd5cd3 Mon Sep 17 00:00:00 2001 From: Thomas Barlow Date: Wed, 30 Oct 2024 16:39:58 +0000 Subject: [PATCH 4/6] Revert "Zillion: Remove generate_output and fill_slot_data thread synchronization" This reverts commit e18906b4fc5b5753e85c654678783e0a06aa3e57. --- test/general/test_implemented.py | 2 +- worlds/zillion/__init__.py | 11 ++++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/test/general/test_implemented.py b/test/general/test_implemented.py index 74ae2459faee..e76d539451ea 100644 --- a/test/general/test_implemented.py +++ b/test/general/test_implemented.py @@ -39,7 +39,7 @@ def test_slot_data(self): """Tests that if a world creates slot data, it's json serializable.""" for game_name, world_type in AutoWorldRegister.world_types.items(): # has an await for generate_output which isn't being called - if game_name in {"Ocarina of Time"}: + if game_name in {"Ocarina of Time", "Zillion"}: continue multiworld = setup_solo_multiworld(world_type) with self.subTest(game=game_name, seed=multiworld.seed): diff --git a/worlds/zillion/__init__.py b/worlds/zillion/__init__.py index 35817c6b02ad..d5e86bb33292 100644 --- a/worlds/zillion/__init__.py +++ b/worlds/zillion/__init__.py @@ -118,6 +118,8 @@ def flush(self) -> None: """ my_locations: List[ZillionLocation] = [] """ This is kind of a cache to avoid iterating through all the multiworld locations in logic. """ + slot_data_ready: threading.Event + """ This event is set in `generate_output` when the data is ready for `fill_slot_data` """ logic_cache: Union[ZillionLogicCache, None] = None def __init__(self, world: MultiWorld, player: int): @@ -125,6 +127,7 @@ def __init__(self, world: MultiWorld, player: int): self.logger = logging.getLogger("Zillion") self.lsi = ZillionWorld.LogStreamInterface(self.logger) self.zz_system = System() + self.slot_data_ready = threading.Event() def _make_item_maps(self, start_char: Chars) -> None: _id_to_name, _id_to_zz_id, id_to_zz_item = make_id_to_others(start_char) @@ -362,7 +365,12 @@ def finalize_item_locations(self) -> GenData: def generate_output(self, output_directory: str) -> None: """This method gets called from a threadpool, do not use multiworld.random here. If you need any last-second randomization, use self.random instead.""" - gen_data = self.finalize_item_locations() + try: + gen_data = self.finalize_item_locations() + except BaseException: + raise + finally: + self.slot_data_ready.set() out_file_base = self.multiworld.get_out_file_name_base(self.player) @@ -385,6 +393,7 @@ def fill_slot_data(self) -> ZillionSlotInfo: # json of WebHostLib.models.Slot # TODO: tell client which canisters are keywords # so it can open and get those when restoring doors + self.slot_data_ready.wait() assert self.zz_system.randomizer, "didn't get randomizer from generate_early" game = self.zz_system.get_game() return get_slot_info(game.regions, game.char_order[0], game.loc_name_2_pretty) From 5bc738dea1a36fa7ea732dc0aca6c7701b866058 Mon Sep 17 00:00:00 2001 From: Thomas Barlow Date: Wed, 30 Oct 2024 22:03:05 +0000 Subject: [PATCH 5/6] Zillion: Finalize item locations in either generate_output or fill_slot_data `generate_output` and `fill_slot_data` are run in separate threads so there is no guarantee that one runs before the other. Before this patch, the threads were synchronized by making `fill_slot_data` wait for `generate_output`. The problem with doing this is that the test.general.test_implemented.test_slot_data test does not run `generate_output`, so Zillion had to be disabled from the test. This patch changes the synchronization of the threads such that it does not matter which function is run first. Whichever function is run first will run `finalize_item_locations()` and the other function will wait until `finalize_item_locations()` is complete and has been cached in a new instance attribute. Zillion has now been re-enabled for the test.general.test_implemented.test_slot_data test. --- test/general/test_implemented.py | 2 +- worlds/zillion/__init__.py | 35 +++++++++++++++++++++++--------- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/test/general/test_implemented.py b/test/general/test_implemented.py index e76d539451ea..74ae2459faee 100644 --- a/test/general/test_implemented.py +++ b/test/general/test_implemented.py @@ -39,7 +39,7 @@ def test_slot_data(self): """Tests that if a world creates slot data, it's json serializable.""" for game_name, world_type in AutoWorldRegister.world_types.items(): # has an await for generate_output which isn't being called - if game_name in {"Ocarina of Time", "Zillion"}: + if game_name in {"Ocarina of Time"}: continue multiworld = setup_solo_multiworld(world_type) with self.subTest(game=game_name, seed=multiworld.seed): diff --git a/worlds/zillion/__init__.py b/worlds/zillion/__init__.py index d5e86bb33292..0262cbee932d 100644 --- a/worlds/zillion/__init__.py +++ b/worlds/zillion/__init__.py @@ -118,8 +118,13 @@ def flush(self) -> None: """ my_locations: List[ZillionLocation] = [] """ This is kind of a cache to avoid iterating through all the multiworld locations in logic. """ - slot_data_ready: threading.Event - """ This event is set in `generate_output` when the data is ready for `fill_slot_data` """ + finalized_gen_data: Optional[GenData] + """ Finalized generation data needed by `generate_output` directly and by `fill_slot_data` indirectly. """ + item_locations_finalization_lock: threading.Lock + """ + This lock is used in `generate_output` and `fill_slot_data` to ensure synchronized access to `finalized_gen_data`, + so that whichever is run first can finalize the item locations while the other waits. + """ logic_cache: Union[ZillionLogicCache, None] = None def __init__(self, world: MultiWorld, player: int): @@ -127,7 +132,8 @@ def __init__(self, world: MultiWorld, player: int): self.logger = logging.getLogger("Zillion") self.lsi = ZillionWorld.LogStreamInterface(self.logger) self.zz_system = System() - self.slot_data_ready = threading.Event() + self.finalized_gen_data = None + self.item_locations_finalization_lock = threading.Lock() def _make_item_maps(self, start_char: Chars) -> None: _id_to_name, _id_to_zz_id, id_to_zz_item = make_id_to_others(start_char) @@ -308,6 +314,19 @@ def post_fill(self) -> None: self.zz_system.post_fill() + def finalize_item_locations_thread_safe(self) -> GenData: + """ + Call self.finalize_item_locations() and cache the result in a thread-safe manner so that either + `generate_output` or `fill_slot_data` can finalize item locations without concern for which of the two functions + is called first. + """ + # The lock is acquired when entering the context manager and released when exiting the context manager. + with self.item_locations_finalization_lock: + # If generation data has yet to be finalized, finalize it. + if self.finalized_gen_data is None: + self.finalized_gen_data = self.finalize_item_locations() + return self.finalized_gen_data + def finalize_item_locations(self) -> GenData: """ sync zilliandomizer item locations with AP item locations @@ -365,12 +384,7 @@ def finalize_item_locations(self) -> GenData: def generate_output(self, output_directory: str) -> None: """This method gets called from a threadpool, do not use multiworld.random here. If you need any last-second randomization, use self.random instead.""" - try: - gen_data = self.finalize_item_locations() - except BaseException: - raise - finally: - self.slot_data_ready.set() + gen_data = self.finalize_item_locations_thread_safe() out_file_base = self.multiworld.get_out_file_name_base(self.player) @@ -393,7 +407,8 @@ def fill_slot_data(self) -> ZillionSlotInfo: # json of WebHostLib.models.Slot # TODO: tell client which canisters are keywords # so it can open and get those when restoring doors - self.slot_data_ready.wait() + # `self.zz_system.get_game()` requires that item locations have been finalized. + self.finalize_item_locations_thread_safe() assert self.zz_system.randomizer, "didn't get randomizer from generate_early" game = self.zz_system.get_game() return get_slot_info(game.regions, game.char_order[0], game.loc_name_2_pretty) From 77fc36e18e1f3efb0d14a6f82970c3fb01172f77 Mon Sep 17 00:00:00 2001 From: Mysteryem Date: Mon, 11 Nov 2024 00:37:54 +0000 Subject: [PATCH 6/6] Apply suggestions from code review Co-authored-by: Doug Hoskisson --- worlds/zillion/__init__.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/worlds/zillion/__init__.py b/worlds/zillion/__init__.py index 0262cbee932d..5a160cd77999 100644 --- a/worlds/zillion/__init__.py +++ b/worlds/zillion/__init__.py @@ -119,7 +119,7 @@ def flush(self) -> None: my_locations: List[ZillionLocation] = [] """ This is kind of a cache to avoid iterating through all the multiworld locations in logic. """ finalized_gen_data: Optional[GenData] - """ Finalized generation data needed by `generate_output` directly and by `fill_slot_data` indirectly. """ + """ Finalized generation data needed by `generate_output` and by `fill_slot_data`. """ item_locations_finalization_lock: threading.Lock """ This lock is used in `generate_output` and `fill_slot_data` to ensure synchronized access to `finalized_gen_data`, @@ -407,10 +407,7 @@ def fill_slot_data(self) -> ZillionSlotInfo: # json of WebHostLib.models.Slot # TODO: tell client which canisters are keywords # so it can open and get those when restoring doors - # `self.zz_system.get_game()` requires that item locations have been finalized. - self.finalize_item_locations_thread_safe() - assert self.zz_system.randomizer, "didn't get randomizer from generate_early" - game = self.zz_system.get_game() + game = self.finalize_item_locations_thread_safe().zz_game return get_slot_info(game.regions, game.char_order[0], game.loc_name_2_pretty) # end of ordered Main.py calls