Skip to content

Fix execute deoptimisation caused by GraphQLEnumType#1288

Merged
leebyron merged 1 commit intographql:masterfrom
IvanGoncharov:enumDeopt
Mar 29, 2018
Merged

Fix execute deoptimisation caused by GraphQLEnumType#1288
leebyron merged 1 commit intographql:masterfrom
IvanGoncharov:enumDeopt

Conversation

@IvanGoncharov
Copy link
Copy Markdown
Member

@IvanGoncharov IvanGoncharov commented Mar 15, 2018

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

graphql-js/src/type/definition.js

Lines 1098 to 1099 in 70f008e

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:

graphql-js/src/type/definition.js

Line 1130 in 70f008e

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
Copy Markdown
Contributor

leebyron commented Mar 29, 2018

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
@IvanGoncharov IvanGoncharov mentioned this pull request Sep 5, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@IvanGoncharov @leebyron @facebook-github-bot