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.

Sign up for GitHub

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

Jump to bottom

Render source page layout with Askama #109187

Merged
merged 2 commits into from
Mar 22, 2023
Merged

Render source page layout with Askama #109187

merged 2 commits into from
Mar 22, 2023

Conversation

clubby789
Copy link
Contributor

@clubby789 clubby789 commented Mar 15, 2023 •
edited
Loading

I was looking at making code_html render into the buffer instead of in advance, but it turned out to need a pretty big refactor, so starting with rearranging the high-level layout.
Found another approach which required much less changes

cc #108868

@rustbot
Copy link
Collaborator

rustbot commented Mar 15, 2023

r? @GuillaumeGomez

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 15, 2023
notriddle
notriddle requested changes Mar 15, 2023
View reviewed changes
Copy link
Contributor

@notriddle notriddle left a comment

Choose a reason for hiding this comment

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

Please do not silently discard errors without a comment explaining why.

src/librustdoc/html/templates/source.html Outdated Show resolved Hide resolved
src/librustdoc/html/highlight.rs Outdated Show resolved Hide resolved
src/librustdoc/html/highlight.rs Outdated Show resolved Hide resolved
src/librustdoc/html/highlight.rs Outdated Show resolved Hide resolved
src/librustdoc/html/highlight.rs Outdated Show resolved Hide resolved
src/librustdoc/html/highlight.rs Outdated Show resolved Hide resolved
src/librustdoc/html/highlight.rs Outdated Show resolved Hide resolved
src/librustdoc/html/highlight.rs Outdated Show resolved Hide resolved
src/librustdoc/html/highlight.rs Outdated Show resolved Hide resolved
src/librustdoc/html/highlight.rs Outdated Show resolved Hide resolved
GuillaumeGomez
GuillaumeGomez reviewed Mar 16, 2023
View reviewed changes
Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

Other than my 2 comments, looks good to me. 👍

@clubby789 @notriddle
Render source page layout with Askama
102c8fa 
Co-authored-by: Michael Howell <michael@notriddle.com>
@notriddle
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 16, 2023
@bors
Copy link
Contributor

bors commented Mar 16, 2023

⌛ Trying commit 102c8fa with merge 169efdfd0f10bb24ab80a18cc508cdfc62cd489c...

@bors
Copy link
Contributor

bors commented Mar 16, 2023

☀️ Try build successful - checks-actions
Build commit: 169efdfd0f10bb24ab80a18cc508cdfc62cd489c (169efdfd0f10bb24ab80a18cc508cdfc62cd489c)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (169efdfd0f10bb24ab80a18cc508cdfc62cd489c): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.8% [0.8%, 0.8%] 1
Regressions ❌
(secondary)		 1.1% [0.5%, 2.0%] 4
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.8% [0.8%, 0.8%] 1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.9% [0.7%, 1.1%] 4
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)		 -2.2% [-3.1%, -1.2%] 2
All ❌✅ (primary) 0.6% [-0.5%, 1.1%] 5

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.5% [0.4%, 0.5%] 2
Regressions ❌
(secondary)		 5.2% [4.6%, 6.1%] 6
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.5% [0.4%, 0.5%] 2

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 18, 2023
@GuillaumeGomez
Copy link
Member

We have some small regressions but the code improvement is worth it I think.

What do you think @notriddle ?

@clubby789
Copy link
Contributor Author

I wonder if it might be worth adding safe to the line numbers? I didn't add it because it looked a bit noisy but it might be the source of the regression

@GuillaumeGomez
Copy link
Member

Well, that's definitely worth a try.

@GuillaumeGomez
Copy link
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 21, 2023
@bors
Copy link
Contributor

bors commented Mar 21, 2023

⌛ Trying commit 4212c1b with merge 2091bf6dc062c75fd04a804f89148a395fc1c98a...

@bors
Copy link
Contributor

bors commented Mar 21, 2023

☀️ Try build successful - checks-actions
Build commit: 2091bf6dc062c75fd04a804f89148a395fc1c98a (2091bf6dc062c75fd04a804f89148a395fc1c98a)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2091bf6dc062c75fd04a804f89148a395fc1c98a): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 0.6% [0.6%, 0.6%] 1
Improvements ✅
(primary)		 -0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.5% [-0.5%, -0.5%] 1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 1.9% [1.9%, 1.9%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.6% [-0.9%, -0.5%] 4
Improvements ✅
(secondary)		 -3.1% [-4.3%, -1.9%] 2
All ❌✅ (primary) -0.1% [-0.9%, 1.9%] 5

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.7% [-0.9%, -0.5%] 2
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.7% [-0.9%, -0.5%] 2

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 21, 2023
@GuillaumeGomez
Copy link
Member

Seems like the regression was fixed, well done!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 22, 2023

📌 Commit 4212c1b has been approved by GuillaumeGomez

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 22, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 22, 2023
@Dylan-DPC
Rollup merge of rust-lang#109187 - clubby789:askama-source, r=Guillau…
07b2721 
…meGomez

Render source page layout with Askama

~~I was looking at making `code_html` render into the buffer instead of in advance, but it turned out to need a pretty big refactor, so starting with rearranging the high-level layout.~~
Found another approach which required much less changes

cc rust-lang#108868
@Dylan-DPC Dylan-DPC mentioned this pull request Mar 22, 2023
Closed
@Dylan-DPC Dylan-DPC mentioned this pull request Mar 22, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2023
@bors
Auto merge of rust-lang#109496 - Dylan-DPC:rollup-u8rsi3h, r=Dylan-DPC
a266f11 
Rollup of 11 pull requests

Successful merges:

 - rust-lang#100311 (Fix handling of trailing bare CR in str::lines)
 - rust-lang#108997 (Change text -> rust highlighting in sanitizer.md)
 - rust-lang#109179 (move Option::as_slice to intrinsic)
 - rust-lang#109187 (Render source page layout with Askama)
 - rust-lang#109280 (Remove `VecMap`)
 - rust-lang#109295 (refactor `fn bootstrap::builder::Builder::compiler_for` logic)
 - rust-lang#109312 (rustdoc: Cleanup parent module tracking for doc links)
 - rust-lang#109317 (Update links for custom discriminants.)
 - rust-lang#109405 (RPITITs are `DefKind::Opaque` with new lowering strategy)
 - rust-lang#109414 (Do not consider synthesized RPITITs on missing items checks)
 - rust-lang#109435 (Detect uninhabited types early in const eval)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d8543ab into rust-lang:master Mar 22, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 22, 2023
@clubby789 clubby789 mentioned this pull request Mar 26, 2023
Open
25 tasks
@nicklimmm nicklimmm mentioned this pull request Jun 17, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@klensy klensy klensy left review comments

@notriddle notriddle notriddle requested changes

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.70.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@clubby789 @rustbot @notriddle @rust-timer @bors @GuillaumeGomez @klensy