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

Bring back memory optimization of caching lowercased node name #185

Closed

Conversation

stanhu
Copy link
Contributor

@stanhu stanhu commented Jul 24, 2018

In 4.6.2, #175 significantly cut down on memory usage by caching the lowercase
version of the node name. However, as reported in #177, this broke certain
transformers that modified the node name since the name was cached. We can
bring back this optimization by updating the node name only if it has been
changed.

Closes #184

In 4.6.2, rgrove#175 significantly cut down on memory usage by caching the lowercase
version of the node name. However, as reported in rgrove#177, this broke certain
transformers that modified the node name since the name was cached. We can
bring back this optimization by updating the node name only if it has been
changed.

Closes rgrove#184

# It's possible that a transformer modifies the node name, so only
# copy the string again if it has changed.
node_name = node.name.downcase if node.name != node_name
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to account for the possibility that node.name and node_name won't have the same case.

One possible optimization would be to compare the downcased node name with the original name at the start of transform_node! (and whenever the node name changes) and use that knowledge to decide whether we need to do a case-insensitive equality check in future iterations of the loop (probably using casecmp).

My guess is that 99+% of the time this would let us skip using casecmp in the loop.

@rgrove rgrove closed this Nov 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to cache node names across transformers
2 participants