rustdoc: Cleanup `html::render::Context` #82356

camelid · 2021-02-21T03:23:14Z

Move most shared fields to SharedContext (except for cache, which
isn't mutated anyway)
Replace a use of Arc with Rc
Make a bunch of fields private
Add static size assertion for Context
Don't share id_map and deref_id_map

rust-highfive · 2021-02-21T03:23:17Z

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

camelid · 2021-02-21T03:24:45Z

rustdoc: Don't put the renderer in the queue

The key question is whether the testsuite will still pass after this :)

camelid · 2021-02-21T03:44:44Z

test result: FAILED. 119 passed; 299 failed; 2 ignored; 0 measured; 0 filtered out; finished in 30.01s

Seems like a bad sign 🤣

camelid · 2021-02-21T03:45:35Z

src/librustdoc/formats/renderer.rs

        } else if item.name.is_some() {
            prof.generic_activity_with_arg("render_item", &*item.name.unwrap_or(unknown).as_str())
-                .run(|| cx.item(item))?;
+                .run(|| format_renderer.item(item))?;
        }
    }


Why does the code work this way, where it constantly clones the Context?

Because some Context fields are rc, meaning they're not clone so to speak. ;)

Hm, so maybe a way to clean this up could be to make SharedContext the only thing that's actually shared?

GuillaumeGomez · 2021-02-21T14:42:57Z

src/librustdoc/html/render/mod.rs

    /// The map used to ensure all generated 'id=' attributes are unique.
-    id_map: Rc<RefCell<IdMap>>,
+    id_map: RefCell<IdMap>,


I think removing the Rc and Arc is having much more side-effects that you might expect. :)

Right, I have the same comment as before: this is a change in behavior and should probably be dropped.

@jyn514 Still think so?

The diff is confusing: That change (and comment) is old, but the new change from #82424 prevents it from being "outdated" from GitHub's point of view.

Before I was trying to get rid of the cloning of Context and get rid of the shared state in Context, so I removed the Rc and RefCell. Now the code still clones Context, but all the shared mutable state is moved to SharedContext, and id_map isn't shared.

id_map wasn't shared before, either. Quoting from f8bd38d:

I'm not sure why they were shared before. I think the reason id_map
worked as a shared value before is that it is cleared before rendering
each item (in render_item). [...]

Note that id_map currently still has to be cleared because otherwise
child items would inherit the id_map of their parent. I'm hoping to
figure out a way to stop cloning Context, but until then we have to
reset id_map.

I think this thread can be marked as resolved.

Now the code still clones Context, but all the shared mutable state is moved to SharedContext, and id_map isn't shared.

I don't quite follow - isn't id_map shared because the same Context is used everywhere? What do you mean by shared?

Can you link to where Context is cloned? It still seems like a change in behavior to me since cloning will no longer update the same id_map in both places.

id_map is cleared (and was cleared before this change as well) before each item is rendered in render_item, so it wasn't shared before and it still isn't shared now.

id_map is cleared/reset here:

https://github.com/rust-lang/rust/blob/f8bd38d0af4d23f34cf7e6304d8eda1ab4a549b4/src/librustdoc/html/render/mod.rs#L1592-L1596

And Context is cloned here:

https://github.com/rust-lang/rust/blob/f8bd38d0af4d23f34cf7e6304d8eda1ab4a549b4/src/librustdoc/formats/renderer.rs#L81

And here:

https://github.com/rust-lang/rust/blob/f8bd38d0af4d23f34cf7e6304d8eda1ab4a549b4/src/librustdoc/formats/renderer.rs#L101

And used for after_krate here:

https://github.com/rust-lang/rust/blob/f8bd38d0af4d23f34cf7e6304d8eda1ab4a549b4/src/librustdoc/formats/renderer.rs#L110-L111

jyn514 · 2021-02-21T18:13:39Z

This will be really hard to get working with all these changes. I'd strongly suggest separating them into separate PRs so you can find what actually caused the test failures.

jyn514 · 2021-02-21T18:14:24Z

Oh err I didn't realize it was a 30 line diff. In that case, maybe just bisect the test failures with git.

src/librustdoc/formats/renderer.rs

src/librustdoc/html/render/mod.rs

jyn514 · 2021-02-21T18:18:05Z

src/librustdoc/html/render/mod.rs

    /// The map used to ensure all generated 'id=' attributes are unique.
-    id_map: Rc<RefCell<IdMap>>,
+    id_map: RefCell<IdMap>,


Right, I have the same comment as before: this is a change in behavior and should probably be dropped.

camelid · 2021-02-21T18:18:32Z

My guess is the root issue here is that Context has some fields that are shared and some that are not, despite there also being a SharedContext.

src/librustdoc/html/render/mod.rs

camelid · 2021-02-22T01:38:56Z

Huh, it compiled locally. Is Context a different size on CI somehow?

jyn514 · 2021-02-22T01:59:42Z

@camelid the size is target dependent, you need to add #[cfg(target_arch = "x86_64")].

camelid · 2021-02-22T02:32:10Z

@camelid the size is target dependent, you need to add #[cfg(target_arch = "x86_64")].

Will that work even though the assert is done on the host machine?

jyn514 · 2021-02-22T02:41:45Z

I don't understand the question? The size is dependent on the target, not the host. If you cross compile from x86_64 to x86 I'd expect you to get the same error as on CI.

camelid · 2021-02-22T02:56:30Z

Okay, I wasn't sure if std::mem::size_of always returned the size as viewed from the target.

camelid · 2021-02-28T20:55:42Z

I squashed the commits down a bit to remove fixup-esque commits. @GuillaumeGomez ready for review!

Also create issue for removing shared mutable state.

It's cloned a lot, so we don't want it to grow in size unexpectedly. Only run the assert on x86-64 since the size is architecture-dependent.

All the tests passed, so it doesn't seem they need to be shared. Plus they should be item/page-specific. I'm not sure why they were shared before. I think the reason `id_map` worked as a shared value before is that it is cleared before rendering each item (in `render_item`). And then I'm guessing `deref_id_map` worked because it's a hashmap keyed by `DefId`, so there was no overlap (though I'm guessing we could have had issues in the future). Note that `id_map` currently still has to be cleared because otherwise child items would inherit the `id_map` of their parent. I'm hoping to figure out a way to stop cloning `Context`, but until then we have to reset `id_map`.

Reduced from 152 bytes to 88 bytes.

There was no need to clone `id_map` because it was reset before each item was rendered. `deref_id_map` was not reset, but it was keyed by `DefId` and thus was unlikely to have collisions (at least for now). Now we just clone the fields that need to be cloned, and instead create fresh versions of the others.

I don't think the boxing helped performance, in fact I think it potentially made it worse. The data was still being copied, but now it was through a pointer. Thinking about it more, I think boxing might only help when you're passing a big object around by value all the time, rather than the slowdown being that you're cloning it.

camelid · 2021-03-06T04:06:52Z

Wow, that was the longest and trickiest rebase I've ever done 😅

camelid · 2021-03-06T04:12:49Z

@GuillaumeGomez This is ready for review! You may want to take a look at #82356 (comment) and #82356 (comment).

src/librustdoc/html/render/context.rs

GuillaumeGomez · 2021-03-07T15:01:50Z

Thanks!

@bors: r+

bors · 2021-03-07T15:01:51Z

📌 Commit 5b74097 has been approved by GuillaumeGomez

bors · 2021-03-08T03:44:13Z

⌛ Testing commit 5b74097 with merge 931ff787d37676e7c93bbd127dadd0d9564df501...

bors · 2021-03-08T07:59:13Z

💥 Test timed out

rust-log-analyzer · 2021-03-08T08:32:03Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

camelid · 2021-03-09T01:34:21Z

Seems spurious. @bors retry

bors · 2021-03-09T04:33:48Z

⌛ Testing commit 5b74097 with merge 4b9f5cc...

bors · 2021-03-09T07:23:12Z

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 4b9f5cc to master...

camelid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Feb 21, 2021

rust-highfive assigned GuillaumeGomez Feb 21, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 21, 2021

This comment has been minimized.

Sign in to view

camelid commented Feb 21, 2021

View reviewed changes

GuillaumeGomez reviewed Feb 21, 2021

View reviewed changes

jyn514 reviewed Feb 21, 2021

View reviewed changes

camelid marked this pull request as draft February 21, 2021 19:01

camelid force-pushed the render-cleanup branch from 3204c82 to 7dd6b9f Compare February 21, 2021 22:22

This comment has been minimized.

Sign in to view

camelid force-pushed the render-cleanup branch from ec40a39 to 12a886f Compare February 21, 2021 22:42

camelid marked this pull request as ready for review February 21, 2021 23:05

jyn514 reviewed Feb 21, 2021

View reviewed changes

src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved

src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

camelid mentioned this pull request Feb 23, 2021

Don't share id_map and deref_id_map #82424

Closed

camelid force-pushed the render-cleanup branch 2 times, most recently from e471d8d to f8bd38d Compare February 28, 2021 20:55

camelid added 6 commits March 5, 2021 19:39

rustdoc: Make a bunch of fields private

b3d2a37

Also create issue for removing shared mutable state.

rustdoc: Add static size assertion for Context

3bc879e

It's cloned a lot, so we don't want it to grow in size unexpectedly. Only run the assert on x86-64 since the size is architecture-dependent.

Box some fields to reduce Context size

ff39c46

Reduced from 152 bytes to 88 bytes.

camelid force-pushed the render-cleanup branch from 23e916e to 5b74097 Compare March 6, 2021 04:06

GuillaumeGomez reviewed Mar 6, 2021

View reviewed changes

src/librustdoc/html/render/context.rs Show resolved Hide resolved

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 7, 2021

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 8, 2021

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 9, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 9, 2021

bors merged commit 4b9f5cc into rust-lang:master Mar 9, 2021

rustbot added this to the 1.52.0 milestone Mar 9, 2021

camelid deleted the render-cleanup branch March 9, 2021 20:08

camelid mentioned this pull request Mar 12, 2021

Remove RefCell from JsonRenderer.index #82354

Closed

rustdoc: Cleanup html::render::Context #82356

rustdoc: Cleanup html::render::Context #82356

Conversation

camelid commented Feb 21, 2021 • edited Loading

rust-highfive commented Feb 21, 2021

camelid commented Feb 21, 2021

This comment has been minimized.

camelid commented Feb 21, 2021

Choose a reason for hiding this comment

GuillaumeGomez Feb 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelid Feb 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 commented Feb 21, 2021

jyn514 commented Feb 21, 2021

Choose a reason for hiding this comment

camelid commented Feb 21, 2021

This comment has been minimized.

This comment has been minimized.

camelid commented Feb 22, 2021

jyn514 commented Feb 22, 2021

camelid commented Feb 22, 2021

jyn514 commented Feb 22, 2021

camelid commented Feb 22, 2021

camelid commented Feb 28, 2021

camelid commented Mar 6, 2021

camelid commented Mar 6, 2021

GuillaumeGomez commented Mar 7, 2021

bors commented Mar 7, 2021

bors commented Mar 8, 2021

bors commented Mar 8, 2021

rust-log-analyzer commented Mar 8, 2021

camelid commented Mar 9, 2021

bors commented Mar 9, 2021

bors commented Mar 9, 2021

rustdoc: Cleanup `html::render::Context` #82356

rustdoc: Cleanup `html::render::Context` #82356

camelid commented Feb 21, 2021 •

edited

Loading

GuillaumeGomez Feb 21, 2021 •

edited

Loading

camelid Feb 28, 2021 •

edited

Loading