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

Let AST nodes own their String content #1041

Closed
wants to merge 1 commit into from
Closed

Let AST nodes own their String content #1041

wants to merge 1 commit into from

Conversation

Kijewski
Copy link
Contributor

@Kijewski Kijewski commented May 14, 2024

This enables having nodes in the same AST that were produced from multiple source files.

Essentially: %s/&str/RcStr/g

This is a draft. For now, I removed all the parser tests, but will re-add them later.

👍🏻 What I like about this PR: The lifetimes are gone, which makes the code much easier on the eyes.

👎🏻 What I dislike about this PR: The lifetimes are gone, so I have to touch every other line of the source code to remove any 'a.

Do you think the PR is worth investing more time into? I hate that I had to change so many lines; this makes the change extremely un-reviewable and makes it difficult to git blame in the future.

The change is part of #1034. I want to do the inclusions directly in the parsing step, not only when generating code. Just as if you replaced {% include "foo.html" %} with the content of foo.html in the template file.

This enables having nodes in the same AST that were produced from
multiple source files.
@GuillaumeGomez
Copy link
Contributor

If this change, if a template file is updated, will it trigger a new compilation (as it should)?

@djc
Copy link
Owner

djc commented May 15, 2024

Have you benchmarked the performance impact on parsing?

If the goal is to have nodes from different sources in the same AST, it's not obvious to me that this is the best way to get there. At least in theory we can just keep Strings for each source on the stack and keep an AST containing references to any of these sources with the same lifetime, right?

@Kijewski
Copy link
Contributor Author

Have you benchmarked the performance impact on parsing?

I didn't, but I daresay the change is not for free. Rc::clone has a check to guard against integer overflows like loop { forget(rc.clone()); }which adds stack unwinding code.

At least in theory we can just keep Strings for each source on the stack and keep an AST containing references to any of these sources with the same lifetime, right?

Sounds like a good idea! Simply have a Vec<String> of sources. When merging two ASTs, we simply move the sources.

@Kijewski Kijewski closed this May 15, 2024
@Kijewski
Copy link
Contributor Author

Benchmarked using #1043
librustdoc/all          time:   [1.1175 ms 1.1192 ms 1.1212 ms]
                        thrpt:  [12.594 MiB/s 12.618 MiB/s 12.636 MiB/s]
                 change:
                        time:   [+175.67% +177.58% +179.81%] (p = 0.00 < 0.05)
                        thrpt:  [-64.262% -63.974% -63.725%]
                        Performance has regressed.

librustdoc/item_info    time:   [20.457 µs 20.704 µs 20.959 µs]
                        thrpt:  [7.5078 MiB/s 7.6004 MiB/s 7.6921 MiB/s]
                 change:
                        time:   [+218.83% +222.20% +225.66%] (p = 0.00 < 0.05)
                        thrpt:  [-69.293% -68.963% -68.635%]
                        Performance has regressed.

librustdoc/item_union   time:   [95.977 µs 96.918 µs 97.914 µs]
                        thrpt:  [10.081 MiB/s 10.184 MiB/s 10.284 MiB/s]
                 change:
                        time:   [+193.26% +197.01% +200.83%] (p = 0.00 < 0.05)
                        thrpt:  [-66.758% -66.331% -65.901%]
                        Performance has regressed.

librustdoc/page         time:   [498.76 µs 502.96 µs 507.32 µs]
                        thrpt:  [12.206 MiB/s 12.311 MiB/s 12.415 MiB/s]
                 change:
                        time:   [+187.30% +190.59% +194.15%] (p = 0.00 < 0.05)
                        thrpt:  [-66.004% -65.588% -65.194%]
                        Performance has regressed.

librustdoc/print_item   time:   [72.098 µs 72.548 µs 73.047 µs]
                        thrpt:  [12.925 MiB/s 13.014 MiB/s 13.095 MiB/s]
                 change:
                        time:   [+191.57% +195.25% +198.93%] (p = 0.00 < 0.05)
                        thrpt:  [-66.547% -66.130% -65.703%]
                        Performance has regressed.

librustdoc/short_item_info
                        time:   [68.824 µs 69.014 µs 69.236 µs]
                        thrpt:  [13.086 MiB/s 13.128 MiB/s 13.164 MiB/s]
                 change:
                        time:   [+138.68% +148.91% +159.25%] (p = 0.00 < 0.05)
                        thrpt:  [-61.427% -59.824% -58.103%]
                        Performance has regressed.

librustdoc/sidebar      time:   [126.72 µs 126.90 µs 127.14 µs]
                        thrpt:  [9.7062 MiB/s 9.7250 MiB/s 9.7386 MiB/s]
                 change:
                        time:   [+193.94% +198.50% +201.95%] (p = 0.00 < 0.05)
                        thrpt:  [-66.882% -66.499% -65.980%]
                        Performance has regressed.

librustdoc/source       time:   [61.878 µs 62.525 µs 63.239 µs]
                        thrpt:  [11.657 MiB/s 11.790 MiB/s 11.914 MiB/s]
                 change:
                        time:   [+180.80% +185.38% +189.28%] (p = 0.00 < 0.05)
                        thrpt:  [-65.431% -64.958% -64.387%]
                        Performance has regressed.

librustdoc/type_layout_size
                        time:   [36.366 µs 36.568 µs 36.763 µs]
                        thrpt:  [7.3673 MiB/s 7.4065 MiB/s 7.4478 MiB/s]
                 change:
                        time:   [+235.28% +237.93% +240.72%] (p = 0.00 < 0.05)
                        thrpt:  [-70.651% -70.408% -70.175%]
                        Performance has regressed.

librustdoc/type_layout  time:   [168.71 µs 169.83 µs 171.06 µs]
                        thrpt:  [15.738 MiB/s 15.852 MiB/s 15.958 MiB/s]
                 change:
                        time:   [+197.93% +200.75% +203.38%] (p = 0.00 < 0.05)
                        thrpt:  [-67.038% -66.749% -66.435%]
                        Performance has regressed.

@Kijewski Kijewski deleted the pr-rc-str branch May 15, 2024 18:23
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.

3 participants