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

Using OnceCell to avoid re-compiling regex in multiple invocation of function #76817

Closed
tesuji opened this issue Sep 17, 2020 · 8 comments
Closed
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tesuji
Copy link
Contributor

tesuji commented Sep 17, 2020

This is debug-only code so it doesn't matter too much in practice, and I'd guess this regex is so small it compiles nearly instantaneously. I wouldn't object to a PR adding a OnceCell if it didn't add too much noise to this code.

Originally posted by #76775 (comment)

@tesuji

This comment has been minimized.

@rustbot rustbot added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 17, 2020
@hbina
Copy link
Contributor

hbina commented Sep 17, 2020

Which OnceCell should I use? Using std::lazy::OnceCell is currently an unstable feature..?

@Mark-Simulacrum
Copy link
Member

std::lazy::OnceCell is the one to use, yes.

@hbina
Copy link
Contributor

hbina commented Sep 17, 2020

It now complains that this is not safe to be shared across threads, is a lock worth it?

@Mark-Simulacrum
Copy link
Member

@hbina
Copy link
Contributor

hbina commented Sep 17, 2020

Yay, it compiles

diff --git a/compiler/rustc_mir/src/dataflow/framework/graphviz.rs b/compiler/rustc_mir/src/dataflow/framework/graphviz.rs
index 179c471cf48..83cd360b0ec 100644
--- a/compiler/rustc_mir/src/dataflow/framework/graphviz.rs
+++ b/compiler/rustc_mir/src/dataflow/framework/graphviz.rs
@@ -1,6 +1,7 @@
 //! A helpful diagram for debugging dataflow problems.
 
 use std::borrow::Cow;
+use std::lazy::SyncOnceCell;
 use std::{io, ops, str};
 
 use regex::Regex;
@@ -570,6 +571,13 @@ where
     }
 }
 
+macro_rules! regex {
+    ($re:literal $(,)?) => {{
+        static RE: SyncOnceCell<regex::Regex> = SyncOnceCell::new();
+        RE.get_or_init(|| Regex::new($re).unwrap())
+    }};
+}
+
 fn diff_pretty<T, C>(new: T, old: T, ctxt: &C) -> String
 where
     T: DebugWithContext<C>,
@@ -578,7 +586,7 @@ where
         return String::new();
     }
 
-    let re = Regex::new("\u{001f}([+-])").unwrap();
+    let re = regex!("\u{001f}([+-])");
 
     let raw_diff = format!("{:#?}", DebugDiffWithAdapter { new, old, ctxt });
 
diff --git a/compiler/rustc_mir/src/lib.rs b/compiler/rustc_mir/src/lib.rs
index 42717f27384..8d2253b4d82 100644
--- a/compiler/rustc_mir/src/lib.rs
+++ b/compiler/rustc_mir/src/lib.rs
@@ -27,6 +27,7 @@ Rust MIR: a lowered representation of Rust.
 #![feature(trait_alias)]
 #![feature(option_expect_none)]
 #![feature(or_patterns)]
+#![feature(once_cell)]
 #![recursion_limit = "256"]
 
 #[macro_use]

This regex macro looks like it could be reused elswhere.

@tesuji
Copy link
Contributor Author

tesuji commented Sep 17, 2020

The Regex::new is only called once in rustc_mir, I don't think we need to introduce macro for it.

RalfJung added a commit to RalfJung/rust that referenced this issue 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 issue 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 issue 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 issue 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 issue 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 issue 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 issue 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
@tesuji
Copy link
Contributor Author

tesuji commented Sep 27, 2020

Fixed. Closing.

@tesuji tesuji closed this as completed Sep 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants