chore(jsii): cache results of case conversions #3602
Conversation
RomainMuller
force-pushed
the
rmuller/cache-case-conversions
branch
9 times, most recently
from
c5a7bbb to
baaa117
Compare
The `Case` library appears on top of the heavy weights when profiling `jsii` compilation runs on `aws-cdk-lib`, accounting for about `9.6%` of the overall execution time (`6,712` out of `68,935` ms). This is quite significant. Faster libraries performing case conversions exist, however they do not offer the same level of functinoality that `Case` does, and often require knowing which casing the string is presently at, when `Case` performs normalization before conversion (this likely explains why these conversions are somewhat expensive), which is convenient in our use-case. It does not seem to be possible to replace `Case` with a faster implementation, as there does not seem to be any. Running a survey on the usage of all functions from `Case` being used during usch a compilation resulted in the following results: ``` Case.camel: 1881755 cached out of 1893235 (99.39%) - 11480 entries / 164.9 hits per entry Case.pascal: 1890795 cached out of 1909959 (99.00%) - 19164 entries / 99.7 hits per entry Case.snake: 1882773 cached out of 1895359 (99.34%) - 12586 entries / 150.6 hits per entry ``` Since there is a cache hit rate of `99%` and entries are re-used `100` times each on average, it appears that adding a cache on those functions will help alleviate that cost. Indeed, after making this change, `Case` is no longer present above the `100ms` mark in the list of heavy weights produced by the profiler. In order to avoid possibly trading performance for out-of-memory errors, the caches are keyed off of weak references, so that the caches can be garbage collected when under memory pressure.
RomainMuller
force-pushed
the
rmuller/cache-case-conversions
branch
from
baaa117 to
e48a0f9
Compare
|
|
||
| class CacheKey { | ||
| // Storing cache keys as weak references to allow garbage collection if there is memory pressure. | ||
| private static readonly STORE = new Map<any, WeakRef<CacheKey>>(); |
There was a problem hiding this comment.
Not sure I understand this additional cache - why do you need it?
There was a problem hiding this comment.
In order to allow the caches to be garbage collected if under memory pressure, I'm using a WeakMap there, which only accepts object keys (primitives such as string are illegal to use as keys there).
The CacheKey is supposed to be a unique value associated to any given case-conversion function, and in order for the WeakMap to work it's magic, I need to ensure I hold no strong reference to the CacheKey objects (or else they won't be GC'd, meaning the string cache also won't be GC'd). I store the function to WeakRef<CacheKey> association so that I'm able to get the "current" CacheKey for a function, but the WeakRef's deref will return undefined after that key was GC'd, so need to make a new one then.
Makes sense?
There was a problem hiding this comment.
Yeap, after I posted It dawned on me that the WeakRef can only accept, well, refs :)
|
|
||
| class Cache { | ||
| // Cache is indexed on a weak CacheKey so the cache can be purged under memory pressure | ||
| private static readonly CACHES = new WeakMap<CacheKey, Map<string, string>>(); |
There was a problem hiding this comment.
Why is CacheKey an ok type to use in a map? Shouldn't it be hashable? Why can't this just be the function name?
There was a problem hiding this comment.
You must be thinking about Java. Any object can be used as a key of a WeakMap, they will be matched based on object identity (and V8 knows to get a hash for these).
There was a problem hiding this comment.
I was definitely thinking about Java 👍
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
|
Merging (with squash)... |
The
Caselibrary appears on top of the heavy weights when profilingjsiicompilation runs onaws-cdk-lib, accounting for about9.6%ofthe overall execution time (
6,712out of68,935ms). This is quitesignificant.
Faster libraries performing case conversions exist, however they do not
offer the same level of functinoality that
Casedoes, and oftenrequire knowing which casing the string is presently at, when
Caseperforms normalization before conversion (this likely explains why these
conversions are somewhat expensive), which is convenient in our
use-case. It does not seem to be possible to replace
Casewith afaster implementation, as there does not seem to be any.
Running a survey on the usage of all functions from
Casebeing usedduring usch a compilation resulted in the following results:
Since there is a cache hit rate of
99%and entries are re-used100times each on average, it appears that adding a cache on those functions
will help alleviate that cost. Indeed, after making this change,
Caseis no longer present above the
100msmark in the list of heavyweights produced by the profiler.
In order to avoid possibly trading performance for out-of-memory errors,
the caches are keyed off of weak references, so that the caches can be
garbage collected when under memory pressure.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.