Skip to content

(towards 312) Improve performance by adding caching to the tokenizer#336

Merged
arporter merged 5 commits intomasterfrom
312_improve_tokeniser
Jun 9, 2022
Merged

(towards 312) Improve performance by adding caching to the tokenizer#336
arporter merged 5 commits intomasterfrom
312_improve_tokeniser

Conversation

@sergisiso
Copy link
Collaborator

See perf gains in #312

@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #336 (6c4bcb3) into master (e085e76) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #336      +/-   ##
==========================================
+ Coverage   91.27%   91.28%   +0.01%     
==========================================
  Files          36       36              
  Lines       12995    13010      +15     
==========================================
+ Hits        11861    11876      +15     
  Misses       1134     1134              
Impacted Files Coverage Δ
src/fparser/common/splitline.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e085e76...6c4bcb3. Read the comment docs.

@sergisiso
Copy link
Collaborator Author

@arporter This is ready for review.

I see there are other places in fparser that use caching/memoization and it usually implements it manually. I think it would be better to make this decorator used more broadly, but when trying to move it to fparser.two.utils it creates cyclic import dependencies that would need to bring imports into the methods (which I want to avoid). I will look at standardizing the decorator another time, for now the memoization of this function already gives good performance gains.

@sergisiso sergisiso requested a review from arporter June 6, 2022 15:10
@sergisiso sergisiso self-assigned this Jun 6, 2022
Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

All good and a nice solution. I'd not come across 'memoization' before but google tells me it's a real word :-)
Tests all OK. New code is pycodestyle/pylint clean.
On my laptop this reduces the time for the benchmark suggested by @gnikit from 59s to 49s (possibly we should add this benchmark as an example).
All of NEMO-OCE is still parsed fine.
PSyclone works fine with this version of fparser.

@arporter arporter added ready for merge PR is waiting on final CI checks before being merged. and removed under review labels Jun 9, 2022
@arporter arporter merged commit 982812c into master Jun 9, 2022
@arporter arporter deleted the 312_improve_tokeniser branch June 9, 2022 08:35
ZedThree added a commit to PlasmaFAIR/fparser that referenced this pull request Jun 9, 2022
* master: (44 commits)
  stfc#336 update changelog
  stfc#343 update version to 0.0.15
  Update changelog
  stfc#328 update changelog
  Rename `BeginStatement.handle_unknown_item`
  Return `None` from unimplemented function
  Use `f` prefix on all lines of multiline f-string
  Return `None` explicitly from some functions
  Remove redundant `return` statements from the end of functions
  Convert some `str.format()` calls to f-strings
  Remove py2 `unicode` string literal
  Remove py2 `unicode` special handling
  Change type in docstring to py3 style hint
  Remove reference to py2 from comments/docstring
  Revert accidental removal of test case
  Fix wrong variable in error message
  Remove duplicated `str` in `isinstance` argument
  pr stfc#322. Updated changelog ready for merge to master.
  stfc#322 try quoting python versions in GHA yml
  stfc#322 updates for review
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for merge PR is waiting on final CI checks before being merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants