buildClientSchema: build enum type value maps lazily #4424
Conversation
yaacovCR
force-pushed
the
build-enum-lazily
branch
from
31a3c51 to
41c044c
Compare
| ([valueName, valueConfig]) => | ||
| new GraphQLEnumValue(this, valueName, valueConfig), | ||
| ); | ||
| this._values = defineEnumValues.bind(undefined, this, config.values); |
There was a problem hiding this comment.
I'd be surprised if the .bind() method ends up faster than the simpler/shorter/clearer version:
| this._values = defineEnumValues.bind(undefined, this, config.values); | |
| this._values = () => defineEnumValues(this, config.values); |
Both require memory allocation, and traditionally .bind() has ended up slower than just using a new function, though the V8 team may have optimised that difference away by now!
There was a problem hiding this comment.
I could benchmark when I get a chance I think we are just using the convention from elsewhere in the file.
There was a problem hiding this comment.
I'd go with the simpler code if there's no perf difference. My guess is that .bind() was assumed to be more efficient, but I don't think it is.
There was a problem hiding this comment.
Hmmm. It seems in some scenarios to carry a small performance cost. I am wary of microbenchmarks but they are all we have at this point.
There was a problem hiding this comment.
Happy that the v8 team have optimised this! Always struck me as weird that a new function was cheaper in the past. Looks like the relevant V8 fix went out in Node 8.3… so a little while ago now! https://github.com/davidmarkclements/v8-perf?tab=readme-ov-file#partial-application-currying-and-binding
This makes definition of enum values lazy similar to how field/input field definition is lazy. It brings a small performance improvement when using enums in large schemas, as the enum values are only defined when they are actually used
yaacovCR
force-pushed
the
build-enum-lazily
branch
from
41c044c to
51d617e
Compare
The option of passing a thunk was made available by: #4018 .
Always passing the thunk helps contribute to a "lazy" build process, which gives the following performance boost: