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

proc_macro: stop using LEB128 for RPC. #59820

Merged
merged 1 commit into from Apr 13, 2019

Conversation

Projects
None yet
8 participants
@eddyb
Copy link
Member

commented Apr 9, 2019

I'm not sure how much of an improvement this creates, it's pretty tricky to measure.

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

@bors try

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2019

r? @nikomatsakis

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

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

⌛️ Trying commit 6688b03 with merge 20daaaa...

bors added a commit that referenced this pull request Apr 9, 2019

Auto merge of #59820 - eddyb:proc-macro-rpc-opt, r=<try>
proc_macro: stop using LEB128 for RPC.

I'm not sure how much of an improvement this creates, it's pretty tricky to measure.
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

☀️ Try build successful - checks-travis
Build commit: 20daaaa

@nnethercote

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

@rust-timer

This comment has been minimized.

Copy link

commented Apr 9, 2019

Success: Queued 20daaaa with parent 3750348, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented Apr 10, 2019

Finished benchmarking try commit 20daaaa

@nnethercote

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

The comparison URL suggests it's a very slight win. But Cachegrind says it's a big win for webrender_api (in #59650):

  • Before: 22,506,640,384 instructions
  • After: 20,777,560,703 instructions

That's an almost 8% reduction.

I admit I don't understand why this change causes an improvement...

@nnethercote

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Local profiling results:

webrender_api-check
        avg: -4.6%      min: -7.7%      max: -3.2%
webrender_api-opt
        avg: -3.1%      min: -7.3%      max: -0.9%
webrender_api-debug
        avg: -4.0%      min: -6.7%      max: -2.5%
@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

This is a classical "time vs memory" tradeoff: by encoding the original integer's bytes, with no "compression", the CPU has to spend roughly one instruction instead of a loop with several instructions per each 7-bit chunk of the integer, not to mention the branching itself.

Because we're only serializing a fixed number of integers per request/response, the memory overhead is tiny and constant, so LEB128 was never really called for.

Feel free to approve this PR if you think it helps, or get @alexcrichton to review it, I guess.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

I don't mind going either way on this, but I'd be fine waiting for a compelling use case to switch

@nnethercote

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

The profiling results above are compelling enough for me.

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

📌 Commit 6688b03 has been approved by nnethercote

Centril added a commit to Centril/rust that referenced this pull request Apr 12, 2019

Rollup merge of rust-lang#59820 - eddyb:proc-macro-rpc-opt, r=nnether…
…cote

proc_macro: stop using LEB128 for RPC.

I'm not sure how much of an improvement this creates, it's pretty tricky to measure.

bors added a commit that referenced this pull request Apr 13, 2019

Auto merge of #59922 - Centril:rollup-0qmx4jg, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #59781 (Remove check_match from const_eval)
 - #59820 (proc_macro: stop using LEB128 for RPC.)
 - #59846 (clarify what the item is in "not a module" error)
 - #59847 (Error when using `catch` after `try`)
 - #59859 (Suggest removing `?` to resolve type errors.)
 - #59862 (Tweak unstable diagnostic output)
 - #59866 (Recover from missing semicolon based on the found token)
 - #59892 (Impl RawFd conversion traits for WASI TcpListener, TcpStream and UdpSocket)

Failed merges:

r? @ghost

@bors bors merged commit 6688b03 into rust-lang:master Apr 13, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details

@eddyb eddyb deleted the eddyb:proc-macro-rpc-opt branch Apr 14, 2019

@fedochet

This comment has been minimized.

Copy link

commented Apr 24, 2019

@eddyb sorry to bother you so long after this PR is closed, but still: does this change breaks ABI for the procedural macros compiled before this change?
More concretely: is calling procedural macros, compiled with compiler before this update, from code, compiled with compiler after this update (using proc_macro::bridge::client API), will lead to errors and panics?
I am currently experiencing errors during macro expansions, and they are quite hard to debug. The errors I got are something like thread 'main' panicked at 'index 4 out of range for slice of length 1', and I believe they are coming from this changed code

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

@fedochet There is no such thing as ABI stability here, Cargo should be recompiling your proc macros for you.

@fedochet

This comment has been minimized.

Copy link

commented Apr 26, 2019

@eddyb but if I am using precompiled procedural macros and their exported functions outside of compiler and load them by hand (basically to emulate procedural macro expansion)?

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Then you have to emulate what Cargo does and rebuild everything every time the output of rustc -vV changes in any way.

@fedochet

This comment has been minimized.

Copy link

commented Apr 30, 2019

Is there any chance that proc_macro ABI/API will be stabilized? I've seen a note in the rustc sources that in future procedural macros may be implemented as executables of some sort. They are now in fact, but without reliable ABI it's not safe to use them in external tools (my use case, for example, is providing autocompletion in IDE based on procedural macros expansion)

@eddyb

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

@fedochet There are no plans for a stable ABI in Rust. In general, you must recompile everything built by rustc when you change its version (with the sole exception of a staticlib / cdylib with a C API of your choosing).
It's all quite safe as long as you validate rustc -vV the same way Cargo does on every build.

@fedochet

This comment has been minimized.

Copy link

commented May 6, 2019

@eddyb please take a look at #60593 - if this problem is real (i hope it's not just on my machine), it is very strange, because you've suggested that I should use the same compiler to compile everything, and from what I'm experiencing it is the only setup that doesn't work (which is, again, very strange)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.