Patashu / break_eternity.js

A Javascript numerical library to represent numbers as large as 10^^1e308 and as small as 10^-10^^1e308. Sequel to break_infinity.js, designed for incremental games.
MIT License
121 stars 44 forks source link

Add cache for fromString #92

Closed mcpower closed 2 years ago

mcpower commented 2 years ago

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.

Patashu commented 2 years ago

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 https://github.com/Patashu/break_eternity.js/pull/92/commits/dc5e216136fdc6bd3ad4a3882d62e156808ae6a5 what's with the change to pow? Is it a mistake or does it have a good reason?

mcpower commented 2 years ago

Oops, that was a mistake to work around https://github.com/Jacorb90/Prestige-Tree/issues/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.

Patashu commented 2 years ago

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!

mcpower commented 2 years ago

so any time we make an object that we could have reused it's a performance hit.

I think this is closely related to https://github.com/Patashu/break_eternity.js/issues/86. I've tried to work around this internally with 38bcc55a0241315f635656e9eec66053eb91acf1 (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.

Patashu commented 2 years ago

Makes sense, really no surprise that it's hard. What we ACTUALLY want is javascript structs but that might never happen.

mcpower commented 2 years ago

Merge conflict resolved (as a merge commit) and minor mistake fixed.