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

Bound the length of nls completions #2073

Merged
merged 5 commits into from
Oct 22, 2024
Merged

Bound the length of nls completions #2073

merged 5 commits into from
Oct 22, 2024

Conversation

jneem
Copy link
Member

@jneem jneem commented Oct 21, 2024

When there's a big contract, the completions can be very large. This can slow down nls substantially, with most of the time being spent in pretty-printing. The current PrettyPrintCap mechanism isn't useful here, because it does all the slow pretty-printing first and then truncates the output.

This PR adds a crude mechanism to bound the size of pretty prints. With this change, completion in the tf-ncl example is instant. There are probably other places that truncated pretty-printing could be used in nls.

I'm not completely happy with how this implementation threads the bounds through, but I couldn't figure out how else to do it:

  • I could use a wrapper type like BoundedTerm<T: Pretty>(T, SizeBounds) to wrap the things that are actually getting pretty-printed, but it requires lots of changes to the pretty-printer to explicitly pass that state around everywhere
  • I tried keeping the state in the allocator and recursing with smaller "child" allocators, but the lifetimes in DocAllocator don't allow this: since the lifetime of the allocator leaks into the returned DocBuilder, you can't format children using a local allocator.

The current approach with interior mutability was from me trying to work around the second issue.

I also wonder if NickelAllocatorExt needs to be a trait. Maybe we can just use BoundedAllocator (possibly renamed) concretely everywhere?

@jneem jneem requested a review from yannham October 21, 2024 04:16
Copy link
Contributor

github-actions bot commented Oct 21, 2024

🐰 Bencher Report

Branch2073/merge
Testbedubuntu-latest

⚠️ WARNING: The following Measure does not have a Threshold. Without a Threshold, no Alerts will ever be generated!

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds CLI flag.

Click to view all benchmark results
BenchmarkLatencynanoseconds (ns)
fibonacci 10📈 view plot
⚠️ NO THRESHOLD
487,290.00
foldl arrays 50📈 view plot
⚠️ NO THRESHOLD
1,768,000.00
foldl arrays 500📈 view plot
⚠️ NO THRESHOLD
6,859,700.00
foldr strings 50📈 view plot
⚠️ NO THRESHOLD
7,212,800.00
foldr strings 500📈 view plot
⚠️ NO THRESHOLD
62,931,000.00
generate normal 250📈 view plot
⚠️ NO THRESHOLD
48,366,000.00
generate normal 50📈 view plot
⚠️ NO THRESHOLD
2,135,300.00
generate normal unchecked 1000📈 view plot
⚠️ NO THRESHOLD
3,373,100.00
generate normal unchecked 200📈 view plot
⚠️ NO THRESHOLD
770,260.00
pidigits 100📈 view plot
⚠️ NO THRESHOLD
3,200,300.00
pipe normal 20📈 view plot
⚠️ NO THRESHOLD
1,512,700.00
pipe normal 200📈 view plot
⚠️ NO THRESHOLD
10,392,000.00
product 30📈 view plot
⚠️ NO THRESHOLD
838,970.00
scalar 10📈 view plot
⚠️ NO THRESHOLD
1,542,500.00
sum 30📈 view plot
⚠️ NO THRESHOLD
842,330.00
🐰 View full continuous benchmarking report in Bencher

Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

I agree that I don't see a much better way to do it, without butchering the actual pretty-printing code. I also think that the NickelAllocatorExt becomes useless - it's was done only because we wanted to monkey-patch the external type BoxAllocator, but now we can just implement the methods directly on our own type.

core/src/pretty.rs Outdated Show resolved Hide resolved
core/src/pretty.rs Outdated Show resolved Hide resolved
core/src/pretty.rs Outdated Show resolved Hide resolved
@jneem
Copy link
Member Author

jneem commented Oct 22, 2024

Ok, the new version gets rid of the NickelAllocatorExt trait and a bunch of generics. It also uses a closure for scoping and avoids underflows.

I couldn't figure out the purpose of the A parameter. Even within pretty, it just seems to get passed around without doing anything. So I got rid of it also.

@jneem jneem requested a review from yannham October 22, 2024 02:47
@jneem jneem force-pushed the shorter-completions branch from 6e9eb84 to 2cbdc83 Compare October 22, 2024 08:39
@jneem jneem enabled auto-merge October 22, 2024 08:45
@jneem jneem added this pull request to the merge queue Oct 22, 2024
Merged via the queue into master with commit 6a0d041 Oct 22, 2024
5 checks passed
@jneem jneem deleted the shorter-completions branch October 22, 2024 08:54
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