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: optimize pp-display for a better and faster goal view and query panel #900

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

rtetley
Copy link
Collaborator

@rtetley rtetley commented Sep 11, 2024

This PR aims at making a better pretty printing algo for the goal view.
It is heavily inspired by the Oppen algorithm.
Closes #859
Closes #869
Closes #752
Closes #835

@gares
Copy link
Member

gares commented Sep 11, 2024

Funny that half of the citations in that paper are by Gerard Huet and Gilles Kahn...

This is a total reimplementation of the pp-display library which
uses Oppen's algorithm to format the goals correctly.
It is much much faster and corrects a number of display problems.
@rtetley rtetley marked this pull request as ready for review September 12, 2024 12:29
@rtetley rtetley changed the title feat: Optimize PpString feat: optimize pp-display for a better and faster goal view and query panel Sep 12, 2024
@rtetley rtetley merged commit b5cdc7d into main Sep 12, 2024
24 checks passed
case DisplayType.box:
currentWidth += estimateBoxWidth(childBox, context);
break;
case DisplayType.break:
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't tried out this, but I believe the estimation would be more accurate if

  • a Break with shouldBreak: true resets the current width
  • estimateBoxWidth returns the max width

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure what you mean ?
Currently the estimation of whether to break or not does not happen here but rather in the scanTokenStream function.
The box always has the max width but its only relevant to breaks if a box is a hvBox in which case all its breaks should happen.
Otherwise we use the estimation of a block which happens from break to break.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, now I understand the algorithm. btw it seems the use of estimateBoxWidth in buildTokenStream results in a runtime that is quadratic wrt the depth of box. Maybe that's fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I agree there is definitely a more optimal way to do this. However buildTokenStream is only called once on receiving a ppString and in practice, even on examples that use to take a considerable amount of time, this version speeds everything up by an order of magnitude. I think there will be time later down the road to optimise this further.

@rtetley rtetley deleted the opt-pp branch December 13, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants