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

(feat) fast append for new node #92

Merged
merged 6 commits into from
Mar 9, 2023

Conversation

YoniFeng
Copy link
Contributor

@YoniFeng YoniFeng commented Mar 4, 2023

I presume that building the tree and only then (possibly) mutating/transforming it, is the most common usage pattern (baseless presumption, but also confirmed for existing dependents like pact-reference).
The current Node.Id.append() API isn't optimized for building up a tree from the ground up, where inserted nodes are new (therefore isolated/detached) and many of the checks and operations performed are redundant.

I'd like to suggest adding an API to allow for quicker building up of the tree.
Would love to hear your thoughts.

Note
The initial commits are very crude. Would like to get feedback before proceeding.
I think a cleaner API for users might be some sort of append_value, where the T value is supplied as an argument. Less verbose and hides the complexity (required assumptions).

@Boshen
Copy link

Boshen commented Mar 5, 2023

Background: The use case for my project is to build up a parent pointing tree in the semantic analyzer:

And after some profiling, we discovered some performance issues so hence this PR.

@YoniFeng
Copy link
Contributor Author

YoniFeng commented Mar 5, 2023

I benchmarked locally with the latest 'fix' commit:

group main throughput speed (MB/s) pr throughput speed (MB/s)
semantic/babylon.max.js 2.18 178.1±42.13ms 58.0 MB/sec 1.00 81.8±1.74ms 126.2 MB/sec
semantic/d3.js 1.47 10.9±0.12ms 50.1 MB/sec 1.00 7.4±0.19ms 73.8 MB/sec
semantic/lodash.js 2.59 2.0±0.02ms 256.2 MB/sec 1.00 774.0±19.45µs 664.1 MB/sec
semantic/pdf.js 2.85 6.6±0.07ms 60.7 MB/sec 1.00 2.3±0.08ms 173.2 MB/sec
semantic/typescript.js 1.51 86.1±1.00ms 111.7 MB/sec 1.00 57.1±1.49ms 168.5 MB/sec

@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #92 (6da73a8) into main (303f900) will increase coverage by 0.5%.
The diff coverage is 100.0%.

❗ Current head 6da73a8 differs from pull request most recent head a8d2178. Consider uploading reports for the commit a8d2178 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##            main     #92     +/-   ##
=======================================
+ Coverage   63.4%   64.0%   +0.5%     
=======================================
  Files          8       8             
  Lines        471     470      -1     
  Branches     149     149             
=======================================
+ Hits         299     301      +2     
+ Misses        52      49      -3     
  Partials     120     120             

@YoniFeng
Copy link
Contributor Author

YoniFeng commented Mar 7, 2023

@saschagrunert gentle ping

Distilled questions:

  1. Thoughts on adding a fast path for a new node? (yay/nay)
  2. Do you have a preference between adding pub fn append_new_node<T>(self, new_child: NodeId, arena: &mut Arena<T>) (current commit/draft) or something like pub fn append_new_value<T>(self, new_value: T, arena: &mut Arena<T>) to the exposed API?

@saschagrunert
Copy link
Owner

Hey @YoniFeng, thank you for the PR! I generally think that it would be useful to extend the API to save some lines in user code. I'm happy with append_new_value (or append_value). Do you think we still need append_new_node here?

@YoniFeng
Copy link
Contributor Author

YoniFeng commented Mar 8, 2023

Only one is needed, I commit just to have both visible.
I'll rename to append_value and remove append_new_node.

src/id.rs Outdated Show resolved Hide resolved
@YoniFeng
Copy link
Contributor Author

YoniFeng commented Mar 8, 2023

PTAL / ready for submit AFAICT

Do I need to squash commits?

@saschagrunert
Copy link
Owner

LGTM, thank you!

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.

None yet

3 participants