Use measureme in self profiler

[wesleywiser]

r? @michaelwoerister

Changes are still very rough.

I'm not sure what the right way to add the measureme dependency is. Currently it's just added with a relative path which Works On My Machine ™️.

I'm also not sure what to do with the category data.

[michaelwoerister]

Looks like a good start! Eventually I'd like to have query names instead of dep-node names here but I don't know how to exactly do that yet.

Have you done performance tests on Windows? If the mmap-sink performance worse than the file-sink there, I think I'd like to get rid of the memmap dependency and implement the mmap-sink only on Unix via direct libc calls (we'll need some stuff from libc that memmap doesn't support anyway). On Windows we'd just use the file-sink.

For now, I think we should just ignore the category data. Later we could encode it in the event_id if we wanted to.

I think we can just publish measureme on crates.io and have lots of major version changes. The other option would be a git dependency. But that's something we should discuss with wg-meta.

[michaelwoerister]

For reference: https://rust-lang.zulipchat.com/#narrow/stream/185694-t-compiler.2Fwg-meta/topic/out-of-tree.20crates.20-.20crates.2Eio.20or.20git.20dependency

[wesleywiser]

Looks like a good start! Eventually I'd like to have query names instead of dep-node names here but I don't know how to exactly do that yet.

Originally I passed Query<'tcx>s into the profiler methods but I couldn't figure out how to preregister the query names so I had to use a FxHashSet to keep track of which query names had been interned into the string table and which hadn't. Maybe the overhead of that could be reduced more but removing it in favor of using dep-node names and pre-registering the names made a noticeable difference in performance.

Have you done performance tests on Windows?

I've been working on getting rustc to build on my Windows box. It's been a long time since I used it and my msvc toolchain was completely out of date. I think that's resolved now and I'm just waiting for LLVM to build. I should have the performance data tonight.

For now, I think we should just ignore the category data.

Ok that's fine. I'll remove the other unused _category parameters as well.

I think we can just publish measureme on crates.io and have lots of major version changes. The other option would be a git dependency. But that's something we should discuss with wg-meta.

I'd personally vote for publishing on crates.io

[wesleywiser]

Results of testing on Windows:

Test Case	Time Taken
regex rustc	5.02s
regex rustc + -Z self-profileFileSerializationSink	5.92s
regex rustc + -Z self-profileMmapSerializationSink	6.04s

[michaelwoerister]

Great, thanks for getting those numbers. Let's use the FileSerializationSink on Windows then.

[michaelwoerister]

Eventually I'd like to have query names instead of dep-node names here but I don't know how to exactly do that yet.

It looks like a recent PR fixed this for us by making DepKind variants and query names match up :)

[wesleywiser]

Oh, wonderful! I can make that change tonight. Do we have consensus on how to add the measureme dependency? I think that's the last thing that needs a fixup before this can be merged.

[michaelwoerister]

Let's go via crates.io, I'd say.

[michaelwoerister]

Note that Q::to_dep_node() is potentially expensive and in non-incremental builds it's not called at all, so we'll have to cache the query name StringId afterall. Ideally it would be created at the same time as the query caches and stored in a field somewhere.

[wesleywiser]

@michaelwoerister Do you like the approach in edff02e better?

[michaelwoerister]

I do -- but it still has some runtime overhead for cloning the query key. Could you try to add an associated constant to QueryConfig that holds the right QueryName variant? Then we could pass the QueryName directly to the record methods. You could maybe even replace the current NAME constant (not sure if it's used anywhere else).

[wesleywiser]

Oh yeah, that's a better idea

[michaelwoerister]

I left a few comments. Looks like this is almost ready to merge.

[wesleywiser]

Thanks for the feedback @michaelwoerister! I've made those changes, pushed, and marked this PR as ready for review.

[wesleywiser]

@bors r=michaelwoerister

[bors]

📌 Commit 56e434d has been approved by michaelwoerister

[bors]

⌛ Testing commit 56e434d with merge 896c3a5...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: michaelwoerister
Pushing 896c3a5 to master...