Skip to content

Replace AtomicUsize with Cell<usize> in the recursion counter#1098

Merged
alamb merged 1 commit intoapache:mainfrom
wzzzzd:cell
Jan 24, 2024
Merged

Replace AtomicUsize with Cell<usize> in the recursion counter#1098
alamb merged 1 commit intoapache:mainfrom
wzzzzd:cell

Conversation

@wzzzzd
Copy link
Copy Markdown
Contributor

@wzzzzd wzzzzd commented Jan 17, 2024

This is a simple PR that eliminates the weird AtomicUsize.

Closes #1097

Copy link
Copy Markdown
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @wzzzzd -- I am not familiar with the relative performance characteristics of Cell vs AtomicUsize -- I wonder did you see the atomic increments show up in a performance trace?

I think there are likely far more heavy weight operations (like string copying) that dominate most sql parsing operations

@coveralls
Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 7555548027

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 87.859%

Totals Coverage Status
Change from base Build 7542509865: 0.001%
Covered Lines: 18851
Relevant Lines: 21456

💛 - Coveralls

@wzzzzd
Copy link
Copy Markdown
Contributor Author

wzzzzd commented Jan 23, 2024

Thank you for the contribution @wzzzzd -- I am not familiar with the relative performance characteristics of Cell vs AtomicUsize -- I wonder did you see the atomic increments show up in a performance trace?

I think there are likely far more heavy weight operations (like string copying) that dominate most sql parsing operations

I wrote a microbenchmark to compare the performance of Cell vs AtomicUsize in a deeply nested query, and the result shows that their difference is negligible (<5%) :(
Nevertheless, I think Cell should be preferred from the perspective of code style, since it does not indicate parallelism or concurrency (sql parsing is purely single-threaded).

@jmhain
Copy link
Copy Markdown
Contributor

jmhain commented Jan 24, 2024

+1 for this change, I was going to open a PR myself until I found this one. Using an atomic here is weird and unnecessary, since Cell exists for exactly this purpose (interior mutability of a value not shared between threads). The Rc wrapping it already makes it so that RecursionCounter and thus Parser are not Send or Sync so this should have no effect on the crate's api either.

Copy link
Copy Markdown
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @wzzzzd and @jmhain

@alamb alamb merged commit 1b6b6aa into apache:main Jan 24, 2024
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.

Replace AtomicUsize with Cell<usize> in the parser's RecursionCounter

4 participants