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

DM-45844: Avoid creating identical parsers #1084

Merged
merged 2 commits into from
Sep 25, 2024
Merged

DM-45844: Avoid creating identical parsers #1084

merged 2 commits into from
Sep 25, 2024

Conversation

andy-slac
Copy link
Contributor

@andy-slac andy-slac commented Sep 25, 2024

Building parsers is expensive, now ParserYacc keeps a cache of parsers,
there is one parser created per combination of keyword parameters. In
reality there will be just one parser because we do not pass any non-default
parameters to constructor. This makes repeated construction or ParserYacc
significantly faster, timing shows reduction from 6.6 sec to 0.3 msec per
1000 instantiations.

This commit removes option for identifier substitution at parser level (idMap
parameter for ParserYacc). We never used that option, its presence made
reusable parsers hard to implement. We handle identifiers in our visitor classes,
so there is no impact on our code. The only use of that option in the unit tests
was also removed.

Also added type annotations to parserLex and parserYacc modules.

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes
  • (if changing dimensions.yaml) make a copy of dimensions.yaml in configs/old_dimensions

Building parsers is expensive, now `ParserYacc` keeps a cache of parsers,
there is one parser created per combination of keyword parameters. In
reality there will be just one parser because we do not pass any non-default
parameters to constructor. This makes repeated construction or `ParserYacc`
significantly faster, timing shows reduction from 6.6 sec to 0.3 msec per
1000 instantiations.

This commit removes option for identifier substitution at parser level (`idMap`
parameter for `ParserYacc`). We never used that option, its presence made
reusable parsers hard to implement. We handle identifiers in our visitor classes,
so there is no impact on our code. The only use of that option in the unit tests
was also removed.
Copy link

codecov bot commented Sep 25, 2024

Codecov Report

Attention: Patch coverage is 94.25287% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.69%. Comparing base (8c62bcf) to head (71576b8).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
.../registry/queries/expressions/parser/parserYacc.py 92.53% 0 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1084      +/-   ##
==========================================
- Coverage   89.69%   89.69%   -0.01%     
==========================================
  Files         360      360              
  Lines       47077    47094      +17     
  Branches     9664     9693      +29     
==========================================
+ Hits        42228    42240      +12     
  Misses       3480     3480              
- Partials     1369     1374       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@dhirving dhirving left a comment

Choose a reason for hiding this comment

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

Looks good to me

@andy-slac andy-slac merged commit ca923c0 into main Sep 25, 2024
18 checks passed
@andy-slac andy-slac deleted the tickets/DM-45844 branch September 25, 2024 16:18
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.

2 participants