refactor(core): introduce cached lazies #11253
Conversation
`Lazy.stringValue()` used to be strictly uncached; however, this has an extremely negative impact on synthesis performance when token resolution is performed many times and complex operations are being performed in Lazies, such as JSONification (see #11250 for context). 99% of the time, the value calculated by a Lazy does not actually need to be recalculated, and the value can simply be cached. Right now, we have no API to indicate whether or not caching on the return value is okay. Although it should *probably* be fine to switch the default behavior of `Lazy.stringValue()` to be cached, it's still a little risky. This PR introduces new methods to construct lazies: * `Lazy.string()`/`Lazy.uncachedString()`. * `Lazy.number()`/`Lazy.uncachedNumber()`. * `Lazy.list()`/`Lazy.uncachedList()`. * `Lazy.any()`/`Lazy.uncachedAny()`. Which replace the existing `Lazy.xxxValue()`. New method names were chosen instead of an additional option for readability: the syntactic structure of how `Lazy.stringValue()` is usually used makes adding a `{ cached: true }` options block to the end of every existing call site rather visually noisy. Plus, deprecating the existing method and replacing it with new ones forces current usage sites to think clearly about whether or not their current use is cacheable or not. The *very* computationally heavy implemention of `toJsonString()` should be changed to be cacheable, but has not been done yet in this PR. This PR simply introduces the concept of cacheable vs uncacheable tokens.
|
|
I thought about it, but not really.
|
| * The inner function will only be invoked one time and cannot depend on | ||
| * resolution context. | ||
| */ | ||
| public static string(producer: IStableStringProducer, options: LazyStringValueOptions = {}) { |
There was a problem hiding this comment.
Aren't we concerned about using
any,string, etc for method names (from a jsii perspective?).
Seems like a valid concern to me, although I see you've done the diligence of checking through our supported languages.
Would it be safe and maybe even clearer if we add preposition to it (asString or toString) indicating which type the Lazy will evaluate into?
I'm ok with leaving as-is if you feel strongly.
There was a problem hiding this comment.
To me, Lazy as String or Lazy to String don't make sense in my head. They imply a conversion, which is not what's happen. What's happening is construction of a new kind of value.
More appropriate would be any of:
Lazy.aString()Lazy.newString()Lazy.makeString()Lazy.someString()Lazy.stringLazy()Lazy.lazyString()
There was a problem hiding this comment.
Your call here. I'm not fussed.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Lazy.stringValue()used to be strictly uncached; however, this has anextremely negative impact on synthesis performance when token resolution
is performed many times and complex operations are being performed in
Lazies, such as JSONification (see #11250 for context).
99% of the time, the value calculated by a Lazy does not actually
need to be recalculated, and the value can simply be cached. Right now,
we have no API to indicate whether or not caching on the return value is
okay.
Although it should probably be fine to switch the default behavior of
Lazy.stringValue()to be cached, it's still a little risky.This PR introduces new methods to construct lazies:
Lazy.string()/Lazy.uncachedString().Lazy.number()/Lazy.uncachedNumber().Lazy.list()/Lazy.uncachedList().Lazy.any()/Lazy.uncachedAny().Which replace the existing
Lazy.xxxValue().New method names were chosen instead of an additional option for
readability: the syntactic structure of how
Lazy.stringValue()isusually used makes adding a
{ cached: true }options block to the endof every existing call site rather visually noisy. Plus, deprecating the
existing method and replacing it with new ones forces current usage
sites to think clearly about whether or not their current use is
cacheable or not.
The very computationally heavy implemention of
toJsonString()shouldbe changed to be cacheable, but has not been done yet in this PR. This
PR simply introduces the concept of cacheable vs uncacheable tokens.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license