Add cache for fromString #92
Conversation
In Prestige Tree Rewritten, Decimal.fromString takes 16% of scripting time, mostly called from the constructor (which is called from D). Strings constants are passed into Decimal methods, which get converted into new Decimals using D. These string constants are being converted into strings every time those methods are called. To fix this, Prestige Tree Rewritten could instead pre-convert all string constants used to be Decimals, then use those Decimals instead. However, that would require a LOT of new code and would be unwieldy to use. Instead, we can implement caching for fromString from break_eternity.js's end. This reduces the aforementioned 16% of scripting time down to 0.6% without changing a line of game code.
This commit also in-lines the type checks from the constructor into D, reducing the need for un-necessary type-checks in the constructor.
See the newly added comment in lru-cache.ts for more information.
Prettier explicitly parenthesised `1 << 10 - 1` to `1 << (10 - 1)`, as bitwise operator precedence is pretty confusing. Thanks Prettier!
|
Makes a lot of sense to cache anything that we do a lot that's expensive, especially given the 'naive' way of writing incremental games leads to doing a lot of things like this. I heard from Razenpok that another thing we'd really like to cache is creating and destroying Decimals, since Javascript has no concept of structs (objects that don't exist on the heap and thus don't need to be heap allocated and garbage collected) so any time we make an object that we could have reused it's a performance hit. I have no idea how hard it would be and what it would involve - would have to look into what incremental games do in practice and optimize for their use case. (Unless you already did it, it's been a hot minute) I have just one question before merging. In dc5e216 what's with the change to pow? Is it a mistake or does it have a good reason? |
|
Oops, that was a mistake to work around Jacorb90/Prestige-Tree#19 when testing. Good catch! Feel free to fix this when merging, or I can fix it in a few hours if you'd prefer that instead. |
|
There's a merge conflict too, so give it a look in a few hours and get it set up. Will merge after that. Thanks! |
I think this is closely related to #86. I've tried to work around this internally with 38bcc55 (D/fromValue_noAlloc returns the same instance of the object if it is cached) in this specific optimisation, but we currently don't nicely expose an interface to minimise allocations/GCs with mutable objects. |
|
Makes sense, really no surprise that it's hard. What we ACTUALLY want is javascript structs but that might never happen. |
dc5e216 introduced an unintended functional change to Decimal.pow intended to be used for testing only.
|
Merge conflict resolved (as a merge commit) and minor mistake fixed. |
In Prestige Tree Rewritten, Decimal.fromString takes 16% of scripting time, mostly called from the constructor (which is called from D). Strings constants are passed into Decimal methods, which get converted into new Decimals using D.
These string constants are being converted into strings every time those methods are called. To fix this, Prestige Tree Rewritten could instead pre-convert all string constants used to be Decimals, then use those Decimals instead. However, that would require a LOT of new code and would be unwieldy to use.
Instead, we can implement caching for fromString from break_eternity.js's end. This reduces the aforementioned 16% of scripting time down to 0.6% without changing a line of game code.
This PR introduces a simple LRU cache that I whipped up. It has the ability to change size on-demand by setting
Decimal.fromStringCache.maxSize, but we currently don't expose this to users.The default cache size is mostly arbitrary (1023) and could be changed to another (power of 2) - 1.