Conversation

AstraLuma

Add standard dunder attributes to (almost) all instances of __slots__

Using the test suite as a benchmark, this change produced no noticeable slow down.

Closes #81

@AstraLumaAstraLuma requested a review from Cito as a code owner March 17, 2020 17:25
@AstraLuma

I did not touch src/graphql/validation/rules/overlapping_fields_can_be_merged.py

@CitoCito requested a review from syrusakbary March 17, 2020 20:59
@Cito

I did not touch src/graphql/validation/rules/overlapping_fields_can_be_merged.py

Good, that PairSet class is only for internal usage anyway.

@Cito

Thanks for sending a PR. Not quite sure if we really should add these to all classes, like Location (attached to every node) and make Token (mainly for internal use in the lexer) extendible.
Would like to get @syrusakbary's opinion regarding this PR.

@Cito

In order to get a better feeling for the memory penalty, I ran memory_profiler while parsing the big_schema_sdl now, and found that for each of the 3 classes in the ast module, it uses 4% more memory, 12% more in total. I think this is pretty significant already. So I suggest relaxing only the Node class where it makes most sense. What do you think?

@AstraLuma

Sure, I can do that. My use case is primarily about Node anyway.

@AstraLuma

How do I do the memory profiling?

@Cito

What I did was loading the _schema, then running parse() on it, and observed the memory increase in that line using the memory_profiler. You can also try with other functions. But of course there must be many nodes in the game.

@AstraLuma

Ok, some variations. Using this script, each is an average of 3 sequential runs

  • master: 42.2MB / 17.8MB
  • This PR, as-is: 44.6MB / 19.9MB
  • Variation 1: 43.1MB / 18.5MB
  • Variation 2: 43.9 MB / 19.3MB
  • Variation 3: 43.9MB / 19.3MB
  • Variation 4: 44.3MB / 19.8MB

Ok, variations:

  • Variation 1: Dropped changes to Location and Token
  • Variation 2: Dropped changes to Token
  • Variation 3: Dropped __dict__ on Location and Token
  • Variation 4: Dropped __dict__ on Token

I was surprised by the memory impact, even between master and variation 3 (presence or removal of __weakref__). All that memory is just space from the extra slots--__weakref__ is initialized to None, which is a singleton. (Under the hood in CPython, it's an actual value, PyNone; PyObject* is not normally NULL.)

I guess I'll go with variation 1, then.

@Cito

Thanks a lot for the detailed verification. I agree variation 1 is the best compromize.

This should decrease the memory impact of this PR from +11% to +4%.
@AstraLuma

Ok, I think that's all squared away.

@CitoCito removed the request for review from syrusakbary March 18, 2020 11:31
@CitoCito changed the title Fix slots Make nodes extensible and weak-referencable Mar 18, 2020
@CitoCito merged commit faa80af into graphql-python:master Mar 18, 2020
@AstraLumaAstraLuma deleted the fix-slots branch March 18, 2020 17:25
Sign up for free to join this conversation on . Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

Weakref unsupported in AST