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

Allow rendering borrowed events #29

Merged
merged 8 commits into from
Mar 25, 2023
Merged

Allow rendering borrowed events #29

merged 8 commits into from
Mar 25, 2023

Conversation

hellux
Copy link
Owner

@hellux hellux commented Mar 19, 2023

addressing #24

benchmarks

both criterion and iai seem to agree there is some loss in performance. note that iai is parse + render and criterion is just the html rendering.

commit                                                          Mevents/s (criterion)   total estimated cycles (iai)
7063b9592 html: avoid peek of next event                        35.068 -14.193%         8331258 (+0.842470%)
38e9574df html: rm FilteredEvents                               37.792 +7.6143%         8341417 (+0.121938%)
ecaea39a4 html: rm events/out from Writer                       37.988 +0.5571%         8331315 (-0.121107%)
245a6a1fe html: extract Writer::render_{event, epilogue}        37.864 +0.7191%         8445378 (+1.369088%)
08800374d mv push/write examples from html to Render trait      37.826 -0.8853%         8445378 (No change)
4b73c19e1 lib: add Render::render_{event, prologue, epilogue}   37.023 -1.2867%         8448659 (+0.038850%)
5be2de71a lib: add Render::{push, write}_ref                    37.049 -0.2755%         8448583 (-0.000900%)

it seems like the avoid peek commit is the biggest problem. I'm not entirely sure why but I guess it is because we add some checks in the render loop:

     fn write(&mut self) -> std::fmt::Result {
         while let Some(e) = self.events.next() {
+            let prev_paragraph = self.prev_paragraph;
+            self.prev_paragraph = matches!(&e, Event::End(Container::Paragraph));
+            if prev_paragraph
+                && self.footnote_number.is_some()
+                && !matches!(&e, Event::End(Container::Footnote { .. }))
+            {
+                // no need to add href before para close
+                self.out.write_str("</p>")?;
+            }
             match e {
                 Event::Start(c, attrs) => {

maybe there is a better way to do it?

@hellux hellux force-pushed the render_ref branch 3 times, most recently from e75f12e to abeed56 Compare March 19, 2023 21:39
@hellux hellux requested a review from kmaasrud March 19, 2023 21:48
Copy link
Collaborator

@kmaasrud kmaasrud left a comment

Choose a reason for hiding this comment

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

Left some notes.

Re. the performance degradation: You should still be able to go back to the old way, right (footnote_backlink_written,) or is there something in this new trait that makes it unfeasible? Might be worth trying, and seeing if we gain back that performance loss.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@hellux
Copy link
Owner Author

hellux commented Mar 21, 2023

Re. the performance degradation: You should still be able to go back to the old way, right (footnote_backlink_written,) or is there something in this new trait that makes it unfeasible? Might be worth trying, and seeing if we gain back that performance loss.

Previously, we relied on the fact that we could peek ahead and check if the upcoming event was a End(Footnote). But now, we instead take one event at a time instead of the whole iterator, so we can't know what will come next.

I guess it would still be possible to override the push/push_ref functions and add that functionality there.

Or maybe there is something smarter we can do

src/lib.rs Outdated Show resolved Hide resolved
hellux added 3 commits March 21, 2023 20:57
Try to make rendering of each event independent.

The only case where we need to peek is when a backref link should be
added to the last paragraph within a footnote.

Before, when exiting a paragraph, we would peek and add the link before
emitting the close tag if the next event is a the footnote end.

Now, the paragraph end event skips emitting a paragraph close tag if it
is within a footnote. The next event will then always close the
paragraph, and if it is a footnote end, it will add the backref link
before closing.
no longer useful as no peeking is needed, use simple early exit instead
separate input/output from rendering state
hellux added 2 commits March 21, 2023 21:09
They apply more to the Render trait now than the implementation in the
html module
@hellux
Copy link
Owner Author

hellux commented Mar 21, 2023

tried a benchmark just to make sure there is a noticeable benefit to borrowing instead of cloning:

html/readme             time:   [23.802 µs 23.815 µs 23.830 µs]
                        thrpt:  [37.893 Melem/s 37.917 Melem/s 37.938 Melem/s]
html_borrow/readme      time:   [25.326 µs 25.386 µs 25.448 µs]
                        thrpt:  [35.484 Melem/s 35.570 Melem/s 35.655 Melem/s]
html_clone/readme       time:   [29.297 µs 29.333 µs 29.375 µs]
                        thrpt:  [30.740 Melem/s 30.785 Melem/s 30.822 Melem/s]

compared to master:

html/readme             time:   [22.776 µs 22.830 µs 22.923 µs]
                        thrpt:  [39.393 Melem/s 39.553 Melem/s 39.648 Melem/s]
html_clone/readme       time:   [28.785 µs 28.818 µs 28.863 µs]
                        thrpt:  [31.286 Melem/s 31.335 Melem/s 31.371 Melem/s]

so, we are 15% faster for borrowed events but 4% slower for owned events..

hellux added 3 commits March 21, 2023 22:35
derive push/write automatically from these
allow rendering iterators with borrowed events

resolves #24
allow comparing between rendering owned, borrowed or cloned events
@hellux hellux merged commit 0884be2 into master Mar 25, 2023
@hellux hellux deleted the render_ref branch March 26, 2023 18:27
hellux added a commit that referenced this pull request Apr 5, 2023
Merge branch 'render_ref'

closes #29
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