Skip to content

Conversation

@kivikakk
Copy link
Owner

@kivikakk kivikakk commented Oct 7, 2025

Was #616. tfw you accidentally push your branch as main and the PR gets merged!?

imagen

5 errors to go.

  • search for XXX.

@kivikakk
Copy link
Owner Author

kivikakk commented Oct 7, 2025

Currently seeing some superlinear behaviour on appending nodes in our stack smashing tests:

https://github.com/saschagrunert/indextree/blob/d75af59d932916a8d0a8568cb2532234004d790c/indextree/src/id.rs#L707-L709

        if self.ancestors(arena).any(|ancestor| new_child == ancestor) {
            return Err(NodeError::AppendAncestor);
        }

Not sure how I want to handle this yet.

edit: easy fix, this is why append_value exists.

@kivikakk
Copy link
Owner Author

kivikakk commented Oct 7, 2025

If we ignore that little inconvenience, we're at:

[   3.366s] 297 tests run: 227 passed, 70 failed, 1 skipped

@kivikakk
Copy link
Owner Author

kivikakk commented Oct 7, 2025

Noticing that our benchmark script is hiding a lot of variance in process startup/teardown, because we run the program once per Pro Git lang.

@kivikakk
Copy link
Owner Author

kivikakk commented Oct 7, 2025

Benchmark 1: ./bench.sh ./comrak-6af7565
  Time (mean ± σ):     138.0 ms ±   2.3 ms    [User: 118.0 ms, System: 21.6 ms]
  Range (min … max):   133.7 ms … 141.6 ms    25 runs

Benchmark 2: ./bench.sh ./comrak-main
  Time (mean ± σ):     128.5 ms ±   0.6 ms    [User: 111.4 ms, System: 18.8 ms]
  Range (min … max):   127.6 ms … 129.7 ms    25 runs
imagen

@kivikakk
Copy link
Owner Author

Rebased and tests passing (haven't done doc tests yet). It's a lot of churn …

image

and, regrettably, slower than the status quo!

image

indextree spends a fair bit more time than typed_arena making sure it's not tying itself in knots. There's also certainly some inelegant parts I've introduced during the port that make it less than optimal.

It's notable that inline parsing is where the main hit is taken; there's a checked_append necessitated by our use of append_node instead of append_value at the end of Subject::parse_inline which we could avoid with some care, though looking at the codepaths of NodeId::append and NodeId::append_value, I'm not certain how big the improvement will be. It will be an improvement, though.

I'll hack at this a little more before calling the experiment done, but so far it's a probable no-merge. The profile does show we still spend an inordinate amount of time revalidating UTF-8 in main, and that looks low-hanging. We could also do better by allocating some Strings with capacity.

@kivikakk
Copy link
Owner Author

kivikakk commented Oct 18, 2025

Oh, and we were still syntax highlighting in our benchmarks. We had a lot of overhead of Syntect represented there!

edit: fixed in #624.

@kivikakk
Copy link
Owner Author

It's notable that inline parsing is where the main hit is taken; there's a checked_append necessitated by our use of append_node instead of append_value at the end of Subject::parse_inline which we could avoid with some care, though looking at the codepaths of NodeId::append and NodeId::append_value, I'm not certain how big the improvement will be. It will be an improvement, though.

8446dd5: swing and a miss. It's worse! Remember to always profile, folks.

@kivikakk
Copy link
Owner Author

image

Calling it. :) I'll keep picking bits out of this branch but I don't think this is The Way.

@kivikakk kivikakk closed this Oct 18, 2025
@kivikakk kivikakk deleted the push-unlyqtpnmzms branch October 18, 2025 03:40
kivikakk added a commit that referenced this pull request Oct 18, 2025
kivikakk added a commit that referenced this pull request Oct 18, 2025
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