Skip to content
New issue

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

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

Already on GitHub? # to your account

memoise wrapper values #1252

Closed
wants to merge 1 commit into from
Closed

Conversation

mohawk2
Copy link
Contributor

@mohawk2 mohawk2 commented Feb 17, 2018

In a similar spirit to #1130, and extracted from code written updating #1054: use WeakMap to cache GraphQLList and GraphQLNonNull wrappers for any given type, so that the wrapper function will always return the same instance. This will make type-comparisons able to be quicker, and avoid constructing unnecessary duplicates.

@leebyron
Copy link
Contributor

Any benchmarking effects from this change, @mohawk2? Conceptually it makes sense, but would love to see real world impact

@mohawk2
Copy link
Contributor Author

mohawk2 commented Feb 28, 2018

@leebyron Surprisingly, the benchmark showed a very very small slowdown. Suspicious, I ran yarn run testonly a number of times on each branch which seemed to show the memoised version was faster by a tiny amount.

I suspected that whichever went second on the benchmark got a speed advantage. I then ran yarn run benchmark --revs local master, and compared it to yarn run benchmark --revs master local. The first one showed roughly similar speed, while the second showed some speedup from memoising.

To summarise the findings:

  • the benchmark favours whichever runs second, and should probably drop the first say 3 runs of each one
  • memoising shows approximately these effects (+ = faster, - = slower):
    • parsing +6%
    • execute introspection well within margin of variation
    • build from introspection +1% but well within margin of variation
    • build from AST -5%

The slowdown in building a schema from AST is not very surprising since memoising trades startup time against runtime.

My compliments to you and @IvanGoncharov for turning my bodgy prototype benchmark into quite a powerful thing! Two questions:

  • would you like me to tweak the benchmark to drop first few runs?
  • do you think adding the "profile" option I put together would add value?

@IvanGoncharov
Copy link
Member

Closing, since in 16.0.0 we allow creating wrappers only by using new.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants