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

Don't compile regex at every function call. #76818

Merged
merged 1 commit into from
Sep 20, 2020

Conversation

hbina
Copy link
Contributor

@hbina hbina commented Sep 17, 2020

Use SyncOnceCell to only compile it once.
I believe this still adds some kind of locking mechanism?

Related issue: #76817

@rust-highfive
Copy link
Collaborator

r? @ecstatic-morse

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 17, 2020
Use `SyncOnceCell` to only compile it once.
I believe this still adds some kind of locking mechanism?
@hbina hbina force-pushed the dont_compile_regex_all_the_time branch from be14ecb to f4a7149 Compare September 17, 2020 03:04
Comment on lines +576 to +577
static RE: SyncOnceCell<regex::Regex> = SyncOnceCell::new();
RE.get_or_init(|| Regex::new($re).unwrap())
Copy link
Contributor

@ecstatic-morse ecstatic-morse Sep 17, 2020

Choose a reason for hiding this comment

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

It seems like this should use SyncLazy instead. @matklad is using Lazy preferable to OnceCell when possible?

Copy link
Member

Choose a reason for hiding this comment

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

Depends on code-style I guess. My preference would be to use OnceCell here, rather than lazy. I generally like to use the smallest tool for the job, and, as we are not really going to use Lazy's deref in any notable capacity here, OnceCell seems preferable.

But I also can see "don't overthink this and use just Lazy" line of thought there :-)

@@ -570,6 +571,13 @@ where
}
}

macro_rules! regex {
Copy link
Contributor

@ecstatic-morse ecstatic-morse Sep 17, 2020

Choose a reason for hiding this comment

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

I quite like this. I think it would be a good addition to the regex crate (behind a feature for now). WDYT @hbina?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have opened an issue here rust-lang/regex#709
I am still new to this so I haven't open a PR yet.

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Sep 17, 2020

Here's a fun thought experiment and the reason I didn't do this earlier. The normal advice for Regex::new is to run it ahead of time and not in a tight loop. However, unlike most calls to Regex::new, this code is rarely executed. So rarely executed that I believe only a single Rust user (me) has ever done it, and I expect to continue to be able to count that number on one hand for the next few years.

While you may care that graphviz diagrams print efficiently on my machine, the vast majority of Rust users don't. In fact, they care so little that what we should probably be optimizing this code for binary size and not runtime efficiency. So while your code is indeed faster, is it smaller? The answer is probably no, although certainly not enough to matter. If anything, your time would actually be better spent making this code slower. For example, I've used argument position impl trait for io::Write when a trait object would do, and the entry point to the graphviz module should probably be #[cold]. Perhaps there's other things I'm missing?

Anyways, if you found any of that compelling feel free to work on it and I'm happy to review. Obviously, code size improvements are very marginal, and we could just merge this (modulo the switch to Lazy).

@hbina
Copy link
Contributor Author

hbina commented Sep 17, 2020

I don't think I have enough expertise to be able to make the analysis nor the call. I would like to run the tests if you could point me to the ways but this is definitely not something I am used to do.

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Sep 17, 2020

No worries. I'll open an issue if I get stuck waiting for something to compile. Let's just merge this as-is. If we get some clarity on the Lazy vs OnceCell issue while this is in the queue it can be updated.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 17, 2020

📌 Commit f4a7149 has been approved by ecstatic-morse

@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 Sep 17, 2020
@rust-lang rust-lang deleted a comment from bors Sep 17, 2020
@rust-lang rust-lang deleted a comment from bors Sep 17, 2020
RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 19, 2020
…me, r=ecstatic-morse

Don't compile regex at every function call.

Use `SyncOnceCell` to only compile it once.
I believe this still adds some kind of locking mechanism?

Related issue: rust-lang#76817
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 19, 2020
…me, r=ecstatic-morse

Don't compile regex at every function call.

Use `SyncOnceCell` to only compile it once.
I believe this still adds some kind of locking mechanism?

Related issue: rust-lang#76817
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 19, 2020
…me, r=ecstatic-morse

Don't compile regex at every function call.

Use `SyncOnceCell` to only compile it once.
I believe this still adds some kind of locking mechanism?

Related issue: rust-lang#76817
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 19, 2020
…me, r=ecstatic-morse

Don't compile regex at every function call.

Use `SyncOnceCell` to only compile it once.
I believe this still adds some kind of locking mechanism?

Related issue: rust-lang#76817
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 20, 2020
…me, r=ecstatic-morse

Don't compile regex at every function call.

Use `SyncOnceCell` to only compile it once.
I believe this still adds some kind of locking mechanism?

Related issue: rust-lang#76817
RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 20, 2020
…me, r=ecstatic-morse

Don't compile regex at every function call.

Use `SyncOnceCell` to only compile it once.
I believe this still adds some kind of locking mechanism?

Related issue: rust-lang#76817
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 20, 2020
Rollup of 15 pull requests

Successful merges:

 - rust-lang#76722 (Test and fix Send and Sync traits of BTreeMap artefacts)
 - rust-lang#76766 (Extract some intrinsics out of rustc_codegen_llvm)
 - rust-lang#76800 (Don't generate bootstrap usage unless it's needed)
 - rust-lang#76809 (simplfy condition in ItemLowerer::with_trait_impl_ref())
 - rust-lang#76815 (Fix wording in mir doc)
 - rust-lang#76818 (Don't compile regex at every function call.)
 - rust-lang#76821 (Remove redundant nightly features)
 - rust-lang#76823 (black_box: silence unused_mut warning when building with cfg(miri))
 - rust-lang#76825 (use `array_windows` instead of `windows` in the compiler)
 - rust-lang#76827 (fix array_windows docs)
 - rust-lang#76828 (use strip_prefix over starts_with and manual slicing based on pattern length (clippy::manual_strip))
 - rust-lang#76840 (Move to intra doc links in core/src/future)
 - rust-lang#76845 (Use intra docs links in core::{ascii, option, str, pattern, hash::map})
 - rust-lang#76853 (Use intra-doc links in library/core/src/task/wake.rs)
 - rust-lang#76871 (support panic=abort in Miri)

Failed merges:

r? `@ghost`
@bors bors merged commit 3268e33 into rust-lang:master Sep 20, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 20, 2020
@hbina hbina deleted the dont_compile_regex_all_the_time branch September 20, 2020 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants