Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix execute deoptimisation caused by GraphQLEnumType #1288

Merged
merged 1 commit into from
Mar 29, 2018

Conversation

IvanGoncharov
Copy link
Member

During profiling of #1286 I notice that completeValue is deoptimize (optimised again after some more calls) by V8 on first serialize call of an enum. See log:

[deoptimizing (DEOPT eager): begin  <JSFunction completeValue (sfi = )> (opt #21) @85, FP to SP delta: 64, caller sp: ]
            ;;; deoptimize at </Users/ivang/dev/graphql-js/perf_test/baseDist/type/definition.js:726:26> inlined at </Users/ivang/dev/graphql-js/perf_test/baseDist/execution/execute.js:1:1>, wrong map
[deoptimizing (eager): end  <JSFunction completeValue (sfi = )> @85 => node=38, pc=, caller sp=, state=TOS_REGISTER, took 2.873 ms]
[deoptimizer unlinked: completeValue / 30d1490930a9]
[deoptimizing (DEOPT lazy): begin  <JSFunction completeValue (sfi = )> (opt #21) @94, FP to SP delta: 64, caller sp: ]
[deoptimizing (lazy): end  <JSFunction completeValue (sfi = )> @94 => node=274, pc=, caller sp=, state=TOS_REGISTER, took 1.624 ms]
[deoptimizing (DEOPT lazy): begin  <JSFunction completeValue (sfi = )> (opt #21) @81, FP to SP delta: 64, caller sp: ]
[deoptimizing (lazy): end  <JSFunction completeValue (sfi = )> @81 => node=426, pc=, caller sp=, state=TOS_REGISTER, took 1.761 ms]
[deoptimizing (DEOPT lazy): begin  <JSFunction completeValue (sfi = )> (opt #21) @94, FP to SP delta: 64, caller sp: ]
[deoptimizing (lazy): end  <JSFunction completeValue (sfi = )> @94 => node=274, pc=, caller sp=, state=TOS_REGISTER, took 1.606 ms]
[marking dependent code  (opt #22) for deoptimization, reason: field-owner]
[marking dependent code  (opt #25) for deoptimization, reason: field-owner]
[deoptimize marked code in all contexts]
[evicting optimizing code marked for deoptimization (unlinking code marked for deopt) for  <SharedFunctionInfo>]
[deoptimizer unlinked: resolveField / 30d149092ef9]
[deoptimize marked code in all contexts]
[deoptimizer unlinked: completeListValue / 30d1490930f1]
[deoptimizing (DEOPT lazy): begin  <JSFunction completeListValue (sfi = )> (opt #48) @13, FP to SP delta: 48, caller sp: ]
[deoptimizing (lazy): end  <JSFunction completeListValue (sfi = )> @13 => node=20, pc=, caller sp=, state=TOS_REGISTER, took 2.938 ms]

Key info from this log is that deoptimization happened at:

deoptimize at </Users/ivang/dev/graphql-js/perf_test/baseDist/type/definition.js:726

serialize(value: any /* T */): ?string {
const enumValue = this._getValueLookup().get(value);

And the reason for deoptimization is field-owner which mean that it was triggered by this assignment:
this._valueLookup = lookup;

It's a problem that limited only to GraphQLEnumType since similar functions from other types (like getTypes, getFields, getInterfaces, etc.) are called by typeMapReducer which in turn always called by the constructor of GraphQLSchema.

Maybe I'm missing something, but I don't see why GraphQLEnumType should be lazy since it doesn't accept Thunk properties in config.

@leebyron
Copy link
Contributor

Nice find. This was lazy to avoid computation and heap space during Schema construction since these enum lookup maps might not be necessary if enum values are never actually requested. Though that may have been a premature optimization. We'll try it out.

A way to fix this while retaining laziness would have been to just assign undefined to those properties during construction so the class shape remains the same.

@leebyron leebyron merged commit 433774d into graphql:master Mar 29, 2018
@IvanGoncharov IvanGoncharov deleted the enumDeopt branch April 1, 2018 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants