Preload data when using Dex.mod #10641

Slayer95 · 2024-10-29T09:05:54Z

With the initial introduction fo TypeScript at 3716f36, Dex.mod, and Dex.format (nowadays Dex.forFormat) no longer ensured that the data cache has been filled.

Newer lazy-loading getter APIs were introduced, which were intended to be used as a replacement for direct access to the loaded data (dataCache).

However, these APIs were either not effective or not properly used., as reflected by 43ef4d9, which reverted Dex.forFormat to its full capabilities.

Nevertheless, Dex.mod was left untouched, which results in a footgun where Dex.forFormat('gen1randbats').gen == 1, but Dex.mod('gen1').gen == 9 - when ran from different processes.

This commit brings back Dex.mod to its original behavior, so that gen information is always there when it's needed.

Thanks to @larry-the-table-guy for reporting this issue.

With the initial introduction fo TypeScript at 3716f36, ``Dex.mod``, and ``Dex.format`` (nowadays ``Dex.forFormat``) no longer ensured that the data cache has been filled. Newer lazy-loading getter APIs were introduced, which were intended to be used as a replacement for direct access to the loaded data (``dataCache``). However, these APIs were either not effective or not properly used., as reflected by 43ef4d9, which reverted ``Dex.forFormat`` to its full capabilities. Nevertheless, ``Dex.mod`` was left untouched, which results in a footgun where ``Dex.forFormat('gen1randbats').gen == 1``, but ``Dex.mod('gen1').gen == 9``. This commit brings back ``Dex.mod`` to its original behavior, so that gen information is always there when it's needed.

Slayer95 · 2024-10-29T09:29:15Z

In theory, assuming gen is the only issue caused by 3716f36, that issue could alternatively be fixed by making dex.gen also an on-demand getter calling loadData() if not cached yet. That would allow to revert 43ef4d9.

But it's pointless to do that, since that would effectively add loadData() to each of the possible code paths. Instead, a sensible lazy approach for dex.gen would be some loadScripts() that handles inheritance. I feel like at some point I wanted to or drafted that, but I don't think the additional complexity added to Dex would be worth it.

urkerab · 2024-10-29T09:50:39Z

Well, there's the code path where you query Dex.gen without loading any data, which still returns 0 at this point; I don't know how useful it would be to make Dex.gen default to 9, although only yesterday I was mildly confused that it didn't, which is the only reason I mention it.

Slayer95 · 2024-10-29T10:14:35Z

I don't know how useful it would be to make Dex.gen default to 9

Maybe one of PS downstream dependant projects could make use of it. I would support adding that default value, for completeness. But it departs a bit from the purpose of this PR. Do you say I should add it here?

urkerab · 2024-10-29T10:25:41Z

I see what you mean about scope creep; I now agree that a separate PR would be better; for instance it would presumably allow the removal of the gen: 9, from the scripts.ts of all of the various gen9 mods.

This is more reliable given the load order.

Slayer95 · 2024-10-29T11:12:38Z

I see what you mean about scope creep; I now agree that a separate PR would be better; for instance it would presumably allow the removal of the gen: 9, from the scripts.ts of all of the various gen9 mods.

🤔 I was originally taking your statement as (roughly) suggesting to (1) add dexes['base'].gen = 9;. But if I we take it as (2) suggesting dexes['base'].includeData(), then that's not as much of "scope screep", as it is something that I feel goes against the lazy loading scheme. So I think that would be controversial.

for instance it would presumably allow the removal of the gen: 9, from the scripts.ts of all of the various gen9 mods.

(3) A default value for this.gen in the Dex constructor? A bit of a twist on (1). Not sure how I feel about that. Not all mods can have their gen trivially bumped when new generations roll out.

Slayer95 · 2024-10-29T11:33:03Z

I feel like (2) is the ideal solution. But only if we are done migrating subprocesses to using Utils instead of Dex for all their non-Pokémon related needs. Otherwise, (1) as stopgap fix. (3) adds more mental burden to new gen developers, and potential issues for side servers, so I feel like it would be a can of worms.

larry-the-table-guy · 2024-10-29T12:29:14Z

I think the root of the problem is the eager loading of mods but lazy loading of data in sim/dex.ts.

IMO the best solution in the long run is to both

don't construct mods until you actually use them
just make the ModdedDex produce a fully constructed object (AKA be normal).

1 requires axing Dex.dexes() and Dex.includeMods() - they have contracts that cannot be implemented both efficiently and correctly.

2 is literally just simpler code. And it makes object de-duplication easier / viable.

There's still plenty of pointless memory in dex, so if lower memory usage was the motivation behind lazy loading, code changes are necessary to achieve that anyway.

We don't need to expose fake data because the real data just isn't that expensive when done right!

larry-the-table-guy · 2024-10-29T12:41:50Z

To better justify my point, here's code that bypasses Dex.mod for Dex.dexes

$ grep -rn "ex.dexes" ./data ./config/ ./server/ ./sim ./test/ ./tools/
./server/chat-plugins/randombattles/index.ts:603:			if (args[1] && toID(args[1]) in Dex.dexes && Dex.dexes[toID(args[1])].gen >= 7) mod = toID(args[1]);
./server/chat-plugins/datasearch.ts:611:		return sanitizedStr.startsWith('mod=') && Dex.dexes[toID(sanitizedStr.split('=')[1])];
./server/chat-plugins/othermetas.ts:21:	if (mod && toID(mod) in Dex.dexes) dex = Dex.mod(toID(mod));
./server/chat-plugins/othermetas.ts:89:			if (toID(mod) in Dex.dexes) {
./server/chat-plugins/othermetas.ts:94:			if (dex === Dex.dexes['gen9ssb']) {
./server/chat-plugins/othermetas.ts:206:			if (toID(sep[1]) in Dex.dexes) {
./server/chat-plugins/othermetas.ts:211:			if (dex === Dex.dexes['gen9ssb']) {
./server/chat-plugins/othermetas.ts:349:		if (args[1] && toID(args[1]) in Dex.dexes) {
./server/chat-plugins/othermetas.ts:350:			dex = Dex.dexes[toID(args[1])];
./server/chat-plugins/othermetas.ts:390:		if (args[1] && toID(args[1]) in Dex.dexes) {
./server/chat-plugins/othermetas.ts:391:			dex = Dex.dexes[toID(args[1])];
./server/chat-plugins/othermetas.ts:603:		if (args[1] && toID(args[1]) in Dex.dexes) {
./server/chat-plugins/othermetas.ts:604:			dex = Dex.dexes[toID(args[1])];
./server/chat-plugins/othermetas.ts:650:		if (mod && toID(mod) in Dex.dexes) {
./server/chat-plugins/othermetas.ts:651:			dex = Dex.dexes[toID(mod)];
./server/chat-plugins/othermetas.ts:701:		if (args[2] && toID(args[2]) in Dex.dexes) {
./server/chat-plugins/othermetas.ts:702:			dex = Dex.dexes[toID(args[2])];
./server/chat.ts:327:		if (toID(formatOrMod) in Dex.dexes) {
./sim/dex-formats.ts:614:			if (!this.dex.dexes[format.mod]) throw new Error(`Format "${format.name}" requires nonexistent mod: '${format.mod}'`);
./sim/dex.ts:13: *   This will preload `Dex.dexes`, giving you a list of possible mods.
./sim/dex.ts:20: *   As above, but will also preload `Dex.dexes[...].data` for all mods.
./test/common.js:33:		if (!Dex.dexes[mod]) throw new Error(`Mod ${mod} does not exist`);

Granted, we could just move them to Dex.mod. But then there's still inconsistency between getting a mod via Dex.mod() and via Dex.dexes[].

Slayer95 · 2024-10-29T13:04:41Z

There's still plenty of pointless memory in dex, so if lower memory usage was the motivation behind lazy loading, code changes are necessary to achieve that anyway.

There is a chronological inaccuracy here. Lazy loading predates the existence of all the Dex(Species|Moves|Items|...) huge caches. All library code, including Utils functions, and toId was imported exclusively from what is now Dex. And that required importing 7 different mods. Lazy loading brought PS memory usage to very slim levels, which made PS very cheap to run in a flourishing ecosystem with multiple popular private servers, from which hail most of PS core contributors past and current.

So things that have changed since then is that there are now many more independent libraries that PS includes.

What I mentioned earlier: we are done migrating subprocesses to using Utils instead of Dex for all their non-Pokémon related needs is a required condition to retire lazy loading. However, ideally also the main process shouldn't need loading unneeded Dex data (*). In fact, the only information it absolutely needs is the list of formats and the list of available mods. Hopefully your plan won't include removing includeFormats(), too, without an equally good alternative.

Granted, we could just move them to Dex.mod. But then there's still inconsistency between getting a mod via Dex.mod() and via Dex.dexes[].

Dex.mod should indeed be the one used by chat plugins (*). I mean, APIs exist for a reason. Dex.dexes are simply internals. Among those lines you listed, there are 2 comment lines (lol), and 3 lines that don't help your case whatsoever. The other lines are from the same chat plugin. Unfortunately, nobody seriously reviews nor cares much about correctness of plugins. So I don't think it's fair to use these as an argument.

Regarding plugins and mods sloppiness, linter rules forbidding many bad patterns, such as direct edition of data without modData() in scripts.ts#.init would be the way to go, imo.

(*) This is according to the assumption that the othermetas chat plugin would be removed in forks.

urkerab · 2024-10-29T13:08:53Z

(3) A default value for this.gen in the Dex constructor? A bit of a twist on (1). Not sure how I feel about that. Not all mods can have their gen trivially bumped when new generations roll out.

Those mods would already have to be changed from gen: 9, to inherit: 'gen9', anyway. (Although maybe a mod named gen9ssb should inherit gen9 by default...)

larry-the-table-guy · 2024-10-29T13:20:07Z

Lazy loading predates huge caches ... Lazy loading brought PS memory usage to very slim levels

I'm sure it made sense at the time. But as the problem changes, the code needs to catch up.

However, ideally also the main process shouldn't need loading unneeded Dex data (*)

If you want some data but not other data, then just separate them? Getting a list of names shouldn't require constructing the objects, lazy-loaded internals or not.

I know I come off as very critical, I just think dex could be much simpler without sacrificing perf. A hierarchy of tables shouldn't be so confusing!

Slayer95 · 2024-10-29T13:28:09Z

I'm sure it made sense at the time. But as the problem changes, the code needs to catch up.

I'd like to keep the lazy loading, and very trivially be able to disable caching to keep my RAM as low as ever. Is that wishful thinking? If it's not, may I add that I would also like PS to start up in 3 seconds, instead of 5+ minutes?

If you want some data but not other data, then just separate them?

I'm not volunteering. Also, I don't mind the critiques. In fact, I am preemptively providing my own critiques for when you enthusiastically PR those improvements 😃

As of PR smogon#10641, these are no longer necessary.

As of PR #10641, these are no longer necessary.

Slayer95 added 2 commits October 29, 2024 04:05

Add test for dex.mod(mod).gen

456c0e0

Swap around test instructions

b03da7f

This is more reliable given the load order.

KrisXV merged commit 36966fa into smogon:master Oct 31, 2024
1 check passed

larry-the-table-guy added a commit to larry-the-table-guy/pokemon-showdown that referenced this pull request Jan 4, 2025

Remove dead code for dex initialization

8e4abf6

As of PR smogon#10641, these are no longer necessary.

larry-the-table-guy mentioned this pull request Jan 4, 2025

Remove dead code for Dex initialization #10793

Merged

KrisXV pushed a commit that referenced this pull request Jan 4, 2025

Remove dead code for dex initialization (#10793)

b790eb8

As of PR #10641, these are no longer necessary.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preload data when using Dex.mod #10641

Preload data when using Dex.mod #10641

Slayer95 commented Oct 29, 2024 •

edited

Loading

Slayer95 commented Oct 29, 2024 •

edited

Loading

urkerab commented Oct 29, 2024

Slayer95 commented Oct 29, 2024 •

edited

Loading

urkerab commented Oct 29, 2024

Slayer95 commented Oct 29, 2024 •

edited

Loading

Slayer95 commented Oct 29, 2024 •

edited

Loading

larry-the-table-guy commented Oct 29, 2024

larry-the-table-guy commented Oct 29, 2024

Slayer95 commented Oct 29, 2024 •

edited

Loading

urkerab commented Oct 29, 2024

larry-the-table-guy commented Oct 29, 2024

Slayer95 commented Oct 29, 2024

Preload data when using Dex.mod #10641

Preload data when using Dex.mod #10641

Conversation

Slayer95 commented Oct 29, 2024 • edited Loading

Slayer95 commented Oct 29, 2024 • edited Loading

urkerab commented Oct 29, 2024

Slayer95 commented Oct 29, 2024 • edited Loading

urkerab commented Oct 29, 2024

Slayer95 commented Oct 29, 2024 • edited Loading

Slayer95 commented Oct 29, 2024 • edited Loading

larry-the-table-guy commented Oct 29, 2024

larry-the-table-guy commented Oct 29, 2024

Slayer95 commented Oct 29, 2024 • edited Loading

urkerab commented Oct 29, 2024

larry-the-table-guy commented Oct 29, 2024

Slayer95 commented Oct 29, 2024

Slayer95 commented Oct 29, 2024 •

edited

Loading

Slayer95 commented Oct 29, 2024 •

edited

Loading

Slayer95 commented Oct 29, 2024 •

edited

Loading

Slayer95 commented Oct 29, 2024 •

edited

Loading

Slayer95 commented Oct 29, 2024 •

edited

Loading

Slayer95 commented Oct 29, 2024 •

edited

Loading