Stream: t-compiler/wg-parallel-rustc

Topic: CrateMetadata source_map_import_info


Santiago Pastorino (Oct 30 2019 at 14:48, on Zulip):

Hey everyone, I was doing a document about CrateMetadata

Santiago Pastorino (Oct 30 2019 at 14:48, on Zulip):

https://hackmd.io/42cKqJroTGKQlNdcID_GlA

Santiago Pastorino (Oct 30 2019 at 14:49, on Zulip):

in particular was going over source_map_import_info field

simulacrum (Oct 30 2019 at 14:49, on Zulip):

s/attribute/field I believe

Santiago Pastorino (Oct 30 2019 at 14:49, on Zulip):

commented to @simulacrum about it, and they've made some comments on the doc

Santiago Pastorino (Oct 30 2019 at 14:49, on Zulip):

we were talking about Once

Santiago Pastorino (Oct 30 2019 at 14:50, on Zulip):

so ... in this case several threads could arrive to the initialization point together

simulacrum (Oct 30 2019 at 14:50, on Zulip):

https://doc.rust-lang.org/nightly/nightly-rustc/rustc_data_structures/sync/struct.Once.html

Santiago Pastorino (Oct 30 2019 at 14:50, on Zulip):

and we want to initialize once and then use it as read only

Santiago Pastorino (Oct 30 2019 at 14:50, on Zulip):

I was wondering how does Once works

simulacrum (Oct 30 2019 at 14:50, on Zulip):

so it looks like once has some methods that would let us do that

Santiago Pastorino (Oct 30 2019 at 14:50, on Zulip):

and if it fits that logic

Santiago Pastorino (Oct 30 2019 at 14:50, on Zulip):

it seemed to me that's exactly what I was looking for :)

simulacrum (Oct 30 2019 at 14:50, on Zulip):

specifically looks like init_locking (https://doc.rust-lang.org/nightly/nightly-rustc/rustc_data_structures/sync/struct.Once.html#method.init_locking) is the right thing here

simulacrum (Oct 30 2019 at 14:51, on Zulip):

Once just wraps a Lock in nice APIs

Santiago Pastorino (Oct 30 2019 at 14:51, on Zulip):

:+1:

Santiago Pastorino (Oct 30 2019 at 14:51, on Zulip):

going to take a look at it and try to port

Santiago Pastorino (Oct 30 2019 at 14:51, on Zulip):

then you mentioned that we may get rid of the sync construction altogether ...

Santiago Pastorino (Oct 30 2019 at 14:51, on Zulip):

any other thought about it?

simulacrum (Oct 30 2019 at 14:51, on Zulip):

I think the primitives in once_cell are actually nicer in this sense

simulacrum (Oct 30 2019 at 14:51, on Zulip):

I think moving to Once first might help

simulacrum (Oct 30 2019 at 14:52, on Zulip):

it'll simplify the code a bit for sure

Santiago Pastorino (Oct 30 2019 at 14:52, on Zulip):

ok, let me move to Once and then we can discuss the rest

simulacrum (Oct 30 2019 at 14:52, on Zulip):

I was going to see if the method is always called in a &mut context

simulacrum (Oct 30 2019 at 14:52, on Zulip):

but I think that's not true?

Santiago Pastorino (Oct 30 2019 at 14:53, on Zulip):

is not

Santiago Pastorino (Oct 30 2019 at 14:53, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_metadata/decoder.rs#L1290-L1374

Santiago Pastorino (Oct 30 2019 at 14:53, on Zulip):

I've realized that my links in the doc point to a specific commit in my local repo, so links don't work on the doc

Santiago Pastorino (Oct 30 2019 at 14:53, on Zulip):

change the sha to master if you need to check something :)

Santiago Pastorino (Oct 30 2019 at 14:53, on Zulip):

sorry about that

simulacrum (Oct 30 2019 at 14:57, on Zulip):

but yeah let's land Once first and I think that's already a good win

Vadim Petrochenkov (Oct 30 2019 at 18:21, on Zulip):

FWIW, I did some review of locks in CrateMetadata previously - https://github.com/rust-lang/rust/pull/65625#issuecomment-544256417.

Santiago Pastorino (Oct 30 2019 at 18:39, on Zulip):

cool gonna go over all these later

Santiago Pastorino (Oct 30 2019 at 18:39, on Zulip):

thanks

Santiago Pastorino (Oct 30 2019 at 19:32, on Zulip):

@simulacrum could jump into this https://github.com/rust-lang/rust/pull/65979

Santiago Pastorino (Oct 30 2019 at 19:33, on Zulip):

left as WIP because I want to see CI running just in case

Santiago Pastorino (Oct 30 2019 at 19:33, on Zulip):

have ran the basic tests but just in case :)

Santiago Pastorino (Oct 30 2019 at 19:34, on Zulip):

so now, about removing the Lock, ideas?

simulacrum (Oct 30 2019 at 19:35, on Zulip):

let's leave it for a future PR

Santiago Pastorino (Oct 30 2019 at 19:36, on Zulip):

yeah that's ok, but I meant, do you have ideas so I can try out

Santiago Pastorino (Oct 30 2019 at 19:36, on Zulip):

I guess we could remove the field from there and use something like CrateMetadataCache structure that holds that info and maybe other caches

Santiago Pastorino (Oct 30 2019 at 19:37, on Zulip):

the problem is that I don't know exactly the lifecycle of the data, where exactly could this be created one time and then reused

simulacrum (Oct 30 2019 at 19:39, on Zulip):

yeah I don't have any without some deeper investigating, sorry -- I've not had time to do so :/

Santiago Pastorino (Oct 30 2019 at 19:45, on Zulip):

no worries

Santiago Pastorino (Oct 30 2019 at 21:09, on Zulip):

@simulacrum just back to this

Santiago Pastorino (Oct 30 2019 at 21:09, on Zulip):

and force pushed

Santiago Pastorino (Oct 30 2019 at 21:10, on Zulip):

check it out just in case there's a better way to do init_locking

Santiago Pastorino (Oct 30 2019 at 21:12, on Zulip):

hmm I think I can just call borrow

Santiago Pastorino (Oct 30 2019 at 21:12, on Zulip):

checking

Santiago Pastorino (Oct 30 2019 at 21:12, on Zulip):

yeah it's exactly the same

Santiago Pastorino (Oct 30 2019 at 21:12, on Zulip):

switching to borrow so I don't need to unwrap :)

Santiago Pastorino (Oct 30 2019 at 21:13, on Zulip):

@simulacrum done

Santiago Pastorino (Oct 31 2019 at 15:57, on Zulip):

@simulacrum the stuff we were talking about yesterday https://github.com/rust-lang/rust/pull/66001

simulacrum (Oct 31 2019 at 15:59, on Zulip):

I've put it in my queue, not sure if I'll have time today :/

Santiago Pastorino (Oct 31 2019 at 15:59, on Zulip):

I try to make commits in a way that's easier to review and not mix things, in this particular the part of moving a method to a separate fn adds a lot of noise and it's in a separate commit :)

Santiago Pastorino (Oct 31 2019 at 15:59, on Zulip):

I've put it in my queue, not sure if I'll have time today :/

no worries

Santiago Pastorino (Oct 31 2019 at 16:00, on Zulip):

the main thing is that this makes source_map_import_info and dep_node_index private

Santiago Pastorino (Nov 01 2019 at 19:11, on Zulip):

@nikomatsakis you wanted to discuss about some refactors related to all the CrateMetadata situation

Santiago Pastorino (Nov 01 2019 at 19:12, on Zulip):

I was telling @nikomatsakis that @Vadim Petrochenkov pointed to some interesting thoughts related to CrateMetadata being behind Lrc and that being why some CrateMetadata fields are behind sync types

Santiago Pastorino (Nov 01 2019 at 19:13, on Zulip):

and that Lrc can't be removed because CrateMetadata needs to be behind it in order to allow CStore (what contains CrateMetadata) be clonable

Santiago Pastorino (Nov 01 2019 at 19:13, on Zulip):

some of the things I've read on github comments are

Santiago Pastorino (Nov 01 2019 at 19:13, on Zulip):
but it actually never happens in rustc, only in rustdoc and perhaps other tools using rustc_interface.
Santiago Pastorino (Nov 01 2019 at 19:14, on Zulip):

talking about cloneing CrateMetadata or CStore I guess

Santiago Pastorino (Nov 01 2019 at 19:14, on Zulip):
    This clone only happens with rustdoc since it needs the resolver available at a later stage. It would be nicer if we could extract the information rustdoc needs instead.
Santiago Pastorino (Nov 01 2019 at 20:29, on Zulip):

@simulacrum I've opened another small PR https://github.com/rust-lang/rust/pull/66024

nikomatsakis (Nov 01 2019 at 20:40, on Zulip):

I guess the question would be to characterize what information is being used from rustdoc

Santiago Pastorino (Nov 01 2019 at 20:49, on Zulip):

yeah, I think so, @Vadim Petrochenkov has also mentioned that perhaps other tools using rustc_interface could be doing the same

Santiago Pastorino (Nov 01 2019 at 20:50, on Zulip):

they mentioned that rustc_interface is hard to reason about, I have no clue about that to be honest, haven't looked at the code

simulacrum (Nov 01 2019 at 20:52, on Zulip):

rustc_interface is trying to put queries where we... are not ready for queries

simulacrum (Nov 01 2019 at 20:53, on Zulip):

@nnethercote had a chart drawn up that looked like what I imagine a modern car engine to

Santiago Pastorino (Nov 01 2019 at 21:18, on Zulip):

@simulacrum btw opened another pr https://github.com/rust-lang/rust/pull/66028

simulacrum (Nov 01 2019 at 21:19, on Zulip):

Yes, I saw -- am hoping to get around to the review of the first one tonight :)

Santiago Pastorino (Nov 01 2019 at 21:20, on Zulip):

@simulacrum what we have discussed about making dependencies private, that we ended with copies now is better in that last one

Santiago Pastorino (Nov 01 2019 at 21:20, on Zulip):

basically there's a method that IMO belongs to CrateMetadata but was sitting on CStore

Santiago Pastorino (Nov 01 2019 at 21:21, on Zulip):

and with that part of the usage of dependencies that made it hard to make it private move away to CrateMetadata :)

nnethercote (Nov 01 2019 at 22:25, on Zulip):

@simulacrum , @Santiago Pastorino : https://github.com/rust-lang/rust/pull/64016#issuecomment-526755156 has the diagrams

nnethercote (Nov 01 2019 at 22:25, on Zulip):

we ended up with something partway between the "old" and "new" pics in that PR

Santiago Pastorino (Nov 02 2019 at 06:12, on Zulip):

@nnethercote are you able to add some of these stuff to rustc-guide? :)

Last update: Nov 17 2019 at 08:05UTC