[Request] [+--- ] D8594: rust: remove support for `re2`
Alphare (Raphaël Gomès)
phabricator at mercurial-scm.org
Fri May 29 10:35:42 UTC 2020
Alphare created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.
REVISION SUMMARY
With the performance issues with `regex` figured out and fixed in previous
patches and `regex` newly gaining support for empty alternations, there is no
reason to keep `re2` around anymore. It's only *marginally* faster at creating
the regex which saves at most a couple of ms, but gets beaten by `regex` in
every other aspect.
This removes the Rust/C/C++ bridge (hooray!), the `with-re2` feature, the
conditional code that goes with it, the documentation and relevant part of the
debug/module output.
REPOSITORY
rHG Mercurial
BRANCH
default
REVISION DETAIL
https://phab.mercurial-scm.org/D8594
AFFECTED FILES
mercurial/debugcommands.py
rust/Cargo.lock
rust/README.rst
rust/hg-core/Cargo.toml
rust/hg-core/build.rs
rust/hg-core/src/lib.rs
rust/hg-core/src/matchers.rs
rust/hg-core/src/re2/mod.rs
rust/hg-core/src/re2/re2.rs
rust/hg-core/src/re2/rust_re2.cpp
rust/hg-cpython/Cargo.toml
rust/hg-cpython/src/debug.rs
tests/test-install.t
CHANGE DETAILS
diff --git a/tests/test-install.t b/tests/test-install.t
--- a/tests/test-install.t
+++ b/tests/test-install.t
@@ -18,7 +18,6 @@
checking available compression engines (*zlib*) (glob)
checking available compression engines for wire protocol (*zlib*) (glob)
checking "re2" regexp engine \((available|missing)\) (re)
- checking "re2" regexp engine Rust bindings \((installed|missing)\) (re) (rust !)
checking templates (*mercurial?templates)... (glob)
checking default template (*mercurial?templates?map-cmdline.default) (glob)
checking commit editor... (*) (glob)
@@ -78,7 +77,6 @@
checking available compression engines (*zlib*) (glob)
checking available compression engines for wire protocol (*zlib*) (glob)
checking "re2" regexp engine \((available|missing)\) (re)
- checking "re2" regexp engine Rust bindings \((installed|missing)\) (re) (rust !)
checking templates (*mercurial?templates)... (glob)
checking default template (*mercurial?templates?map-cmdline.default) (glob)
checking commit editor... (*) (glob)
@@ -126,7 +124,6 @@
checking available compression engines (*zlib*) (glob)
checking available compression engines for wire protocol (*zlib*) (glob)
checking "re2" regexp engine \((available|missing)\) (re)
- checking "re2" regexp engine Rust bindings \((installed|missing)\) (re) (rust !)
checking templates (*mercurial?templates)... (glob)
checking default template (*mercurial?templates?map-cmdline.default) (glob)
checking commit editor... ($TESTTMP/tools/testeditor.exe)
@@ -154,7 +151,6 @@
checking available compression engines (*zlib*) (glob)
checking available compression engines for wire protocol (*zlib*) (glob)
checking "re2" regexp engine \((available|missing)\) (re)
- checking "re2" regexp engine Rust bindings \((installed|missing)\) (re) (rust !)
checking templates (*mercurial?templates)... (glob)
checking default template (*mercurial?templates?map-cmdline.default) (glob)
checking commit editor... (c:\foo\bar\baz.exe) (windows !)
@@ -211,7 +207,6 @@
checking available compression engines (*) (glob)
checking available compression engines for wire protocol (*) (glob)
checking "re2" regexp engine \((available|missing)\) (re)
- checking "re2" regexp engine Rust bindings \((installed|missing)\) (re) (rust !)
checking templates ($TESTTMP/installenv/*/site-packages/mercurial/templates)... (glob)
checking default template ($TESTTMP/installenv/*/site-packages/mercurial/templates/map-cmdline.default) (glob)
checking commit editor... (*) (glob)
@@ -252,7 +247,6 @@
checking available compression engines (*) (glob)
checking available compression engines for wire protocol (*) (glob)
checking "re2" regexp engine \((available|missing)\) (re)
- checking "re2" regexp engine Rust bindings \((installed|missing)\) (re) (rust !)
checking templates ($TESTTMP/installenv/*/site-packages/mercurial/templates)... (glob)
checking default template ($TESTTMP/installenv/*/site-packages/mercurial/templates/map-cmdline.default) (glob)
checking commit editor... (*) (glob)
diff --git a/rust/hg-cpython/src/debug.rs b/rust/hg-cpython/src/debug.rs
--- a/rust/hg-cpython/src/debug.rs
+++ b/rust/hg-cpython/src/debug.rs
@@ -16,8 +16,6 @@
m.add(py, "__package__", package)?;
m.add(py, "__doc__", "Rust debugging information")?;
- m.add(py, "re2_installed", cfg!(feature = "with-re2"))?;
-
let sys = PyModule::import(py, "sys")?;
let sys_modules: PyDict = sys.get(py, "modules")?.extract(py)?;
sys_modules.set_item(py, dotted_name, &m)?;
diff --git a/rust/hg-cpython/Cargo.toml b/rust/hg-cpython/Cargo.toml
--- a/rust/hg-cpython/Cargo.toml
+++ b/rust/hg-cpython/Cargo.toml
@@ -10,7 +10,6 @@
[features]
default = ["python27"]
-with-re2 = ["hg-core/with-re2"]
# Features to build an extension module:
python27 = ["cpython/python27-sys", "cpython/extension-module-2-7"]
diff --git a/rust/hg-core/src/re2/rust_re2.cpp b/rust/hg-core/src/re2/rust_re2.cpp
deleted file mode 100644
--- a/rust/hg-core/src/re2/rust_re2.cpp
+++ /dev/null
@@ -1,49 +0,0 @@
-/*
-rust_re2.cpp
-
-C ABI export of Re2's C++ interface for Rust FFI.
-
-Copyright 2020 Valentin Gatien-Baron
-
-This software may be used and distributed according to the terms of the
-GNU General Public License version 2 or any later version.
-*/
-
-#include <re2/re2.h>
-using namespace re2;
-
-extern "C" {
- RE2* rust_re2_create(const char* data, size_t len) {
- RE2::Options o;
- o.set_encoding(RE2::Options::Encoding::EncodingLatin1);
- o.set_log_errors(false);
- o.set_max_mem(50000000);
-
- return new RE2(StringPiece(data, len), o);
- }
-
- void rust_re2_destroy(RE2* re) {
- delete re;
- }
-
- bool rust_re2_ok(RE2* re) {
- return re->ok();
- }
-
- void rust_re2_error(RE2* re, const char** outdata, size_t* outlen) {
- const std::string& e = re->error();
- *outdata = e.data();
- *outlen = e.length();
- }
-
- bool rust_re2_match(RE2* re, char* data, size_t len, int ianchor) {
- const StringPiece sp = StringPiece(data, len);
-
- RE2::Anchor anchor =
- ianchor == 0 ? RE2::Anchor::UNANCHORED :
- (ianchor == 1 ? RE2::Anchor::ANCHOR_START :
- RE2::Anchor::ANCHOR_BOTH);
-
- return re->Match(sp, 0, len, anchor, NULL, 0);
- }
-}
diff --git a/rust/hg-core/src/re2/re2.rs b/rust/hg-core/src/re2/re2.rs
deleted file mode 100644
--- a/rust/hg-core/src/re2/re2.rs
+++ /dev/null
@@ -1,66 +0,0 @@
-/*
-re2.rs
-
-Rust FFI bindings to Re2.
-
-Copyright 2020 Valentin Gatien-Baron
-
-This software may be used and distributed according to the terms of the
-GNU General Public License version 2 or any later version.
-*/
-use libc::{c_int, c_void};
-
-type Re2Ptr = *const c_void;
-
-pub struct Re2(Re2Ptr);
-
-/// `re2.h` says:
-/// "An "RE2" object is safe for concurrent use by multiple threads."
-unsafe impl Sync for Re2 {}
-
-/// These bind to the C ABI in `rust_re2.cpp`.
-extern "C" {
- fn rust_re2_create(data: *const u8, len: usize) -> Re2Ptr;
- fn rust_re2_destroy(re2: Re2Ptr);
- fn rust_re2_ok(re2: Re2Ptr) -> bool;
- fn rust_re2_error(
- re2: Re2Ptr,
- outdata: *mut *const u8,
- outlen: *mut usize,
- ) -> bool;
- fn rust_re2_match(
- re2: Re2Ptr,
- data: *const u8,
- len: usize,
- anchor: c_int,
- ) -> bool;
-}
-
-impl Re2 {
- pub fn new(pattern: &[u8]) -> Result<Re2, String> {
- unsafe {
- let re2 = rust_re2_create(pattern.as_ptr(), pattern.len());
- if rust_re2_ok(re2) {
- Ok(Re2(re2))
- } else {
- let mut data: *const u8 = std::ptr::null();
- let mut len: usize = 0;
- rust_re2_error(re2, &mut data, &mut len);
- Err(String::from_utf8_lossy(std::slice::from_raw_parts(
- data, len,
- ))
- .to_string())
- }
- }
- }
-
- pub fn is_match(&self, data: &[u8]) -> bool {
- unsafe { rust_re2_match(self.0, data.as_ptr(), data.len(), 1) }
- }
-}
-
-impl Drop for Re2 {
- fn drop(&mut self) {
- unsafe { rust_re2_destroy(self.0) }
- }
-}
diff --git a/rust/hg-core/src/re2/mod.rs b/rust/hg-core/src/re2/mod.rs
deleted file mode 100644
--- a/rust/hg-core/src/re2/mod.rs
+++ /dev/null
@@ -1,21 +0,0 @@
-/// re2 module
-///
-/// The Python implementation of Mercurial uses the Re2 regex engine when
-/// possible and if the bindings are installed, falling back to Python's `re`
-/// in case of unsupported syntax (Re2 is a non-backtracking engine).
-///
-/// Using it from Rust is not ideal. We need C++ bindings, a C++ compiler,
-/// Re2 needs to be installed... why not just use the `regex` crate?
-///
-/// Using Re2 from the Rust implementation guarantees backwards compatibility.
-/// We know it will work out of the box without needing to figure out the
-/// subtle differences in syntax. For example, `regex` currently does not
-/// support empty alternations (regex like `a||b`) which happens more often
-/// than we might think. Old benchmarks also showed worse performance from
-/// regex than with Re2, but the methodology and results were lost, so take
-/// this with a grain of salt.
-///
-/// The idea is to use Re2 for now as a temporary phase and then investigate
-/// how much work would be needed to use `regex`.
-mod re2;
-pub use re2::Re2;
diff --git a/rust/hg-core/src/matchers.rs b/rust/hg-core/src/matchers.rs
--- a/rust/hg-core/src/matchers.rs
+++ b/rust/hg-core/src/matchers.rs
@@ -7,8 +7,6 @@
//! Structs and types for matching files and directories.
-#[cfg(feature = "with-re2")]
-use crate::re2::Re2;
use crate::{
dirstate::dirs_multiset::DirsChildrenMultiset,
filepatterns::{
@@ -239,29 +237,24 @@
}
/// Matches files that are included in the ignore rules.
-#[cfg_attr(
- feature = "with-re2",
- doc = r##"
-```
-use hg::{
- matchers::{IncludeMatcher, Matcher},
- IgnorePattern,
- PatternSyntax,
- utils::hg_path::HgPath
-};
-use std::path::Path;
-///
-let ignore_patterns =
-vec![IgnorePattern::new(PatternSyntax::RootGlob, b"this*", Path::new(""))];
-let (matcher, _) = IncludeMatcher::new(ignore_patterns, "").unwrap();
-///
-assert_eq!(matcher.matches(HgPath::new(b"testing")), false);
-assert_eq!(matcher.matches(HgPath::new(b"this should work")), true);
-assert_eq!(matcher.matches(HgPath::new(b"this also")), true);
-assert_eq!(matcher.matches(HgPath::new(b"but not this")), false);
-```
-"##
-)]
+/// ```
+/// use hg::{
+/// matchers::{IncludeMatcher, Matcher},
+/// IgnorePattern,
+/// PatternSyntax,
+/// utils::hg_path::HgPath
+/// };
+/// use std::path::Path;
+/// ///
+/// let ignore_patterns =
+/// vec![IgnorePattern::new(PatternSyntax::RootGlob, b"this*", Path::new(""))];
+/// let (matcher, _) = IncludeMatcher::new(ignore_patterns, "").unwrap();
+/// ///
+/// assert_eq!(matcher.matches(HgPath::new(b"testing")), false);
+/// assert_eq!(matcher.matches(HgPath::new(b"this should work")), true);
+/// assert_eq!(matcher.matches(HgPath::new(b"this also")), true);
+/// assert_eq!(matcher.matches(HgPath::new(b"but not this")), false);
+/// ```
pub struct IncludeMatcher<'a> {
patterns: Vec<u8>,
match_fn: Box<dyn for<'r> Fn(&'r HgPath) -> bool + 'a + Sync>,
@@ -319,22 +312,6 @@
}
}
-#[cfg(feature = "with-re2")]
-/// Returns a function that matches an `HgPath` against the given regex
-/// pattern.
-///
-/// This can fail when the pattern is invalid or not supported by the
-/// underlying engine `Re2`, for instance anything with back-references.
-#[timed]
-fn re_matcher(
- pattern: &[u8],
-) -> PatternResult<impl Fn(&HgPath) -> bool + Sync> {
- let regex = Re2::new(pattern);
- let regex = regex.map_err(|e| PatternError::UnsupportedSyntax(e))?;
- Ok(move |path: &HgPath| regex.is_match(path.as_bytes()))
-}
-
-#[cfg(not(feature = "with-re2"))]
/// Returns a function that matches an `HgPath` against the given regex
/// pattern.
///
@@ -840,7 +817,6 @@
);
}
- #[cfg(feature = "with-re2")]
#[test]
fn test_includematcher() {
// VisitchildrensetPrefix
diff --git a/rust/hg-core/src/lib.rs b/rust/hg-core/src/lib.rs
--- a/rust/hg-core/src/lib.rs
+++ b/rust/hg-core/src/lib.rs
@@ -23,8 +23,6 @@
pub mod matchers;
pub mod revlog;
pub use revlog::*;
-#[cfg(feature = "with-re2")]
-pub mod re2;
pub mod utils;
// Remove this to see (potential) non-artificial compile failures. MacOS
@@ -141,9 +139,6 @@
/// Needed a pattern that can be turned into a regex but got one that
/// can't. This should only happen through programmer error.
NonRegexPattern(IgnorePattern),
- /// This is temporary, see `re2/mod.rs`.
- /// This will cause a fallback to Python.
- Re2NotInstalled,
}
impl ToString for PatternError {
@@ -166,10 +161,6 @@
PatternError::NonRegexPattern(pattern) => {
format!("'{:?}' cannot be turned into a regex", pattern)
}
- PatternError::Re2NotInstalled => {
- "Re2 is not installed, cannot use regex functionality."
- .to_string()
- }
}
}
}
diff --git a/rust/hg-core/build.rs b/rust/hg-core/build.rs
deleted file mode 100644
--- a/rust/hg-core/build.rs
+++ /dev/null
@@ -1,61 +0,0 @@
-// build.rs
-//
-// Copyright 2020 Raphaël Gomès <rgomes at octobus.net>
-//
-// This software may be used and distributed according to the terms of the
-// GNU General Public License version 2 or any later version.
-
-#[cfg(feature = "with-re2")]
-use cc;
-
-/// Uses either the system Re2 install as a dynamic library or the provided
-/// build as a static library
-#[cfg(feature = "with-re2")]
-fn compile_re2() {
- use cc;
- use std::path::Path;
- use std::process::exit;
-
- let msg = r"HG_RE2_PATH must be one of `system|<path to build source clone of Re2>`";
- let re2 = match std::env::var_os("HG_RE2_PATH") {
- None => {
- eprintln!("{}", msg);
- exit(1)
- }
- Some(v) => {
- if v == "system" {
- None
- } else {
- Some(v)
- }
- }
- };
-
- let mut options = cc::Build::new();
- options
- .cpp(true)
- .flag("-std=c++11")
- .file("src/re2/rust_re2.cpp");
-
- if let Some(ref source) = re2 {
- options.include(Path::new(source));
- };
-
- options.compile("librustre.a");
-
- if let Some(ref source) = &re2 {
- // Link the local source statically
- println!(
- "cargo:rustc-link-search=native={}",
- Path::new(source).join(Path::new("obj")).display()
- );
- println!("cargo:rustc-link-lib=static=re2");
- } else {
- println!("cargo:rustc-link-lib=re2");
- }
-}
-
-fn main() {
- #[cfg(feature = "with-re2")]
- compile_re2();
-}
diff --git a/rust/hg-core/Cargo.toml b/rust/hg-core/Cargo.toml
--- a/rust/hg-core/Cargo.toml
+++ b/rust/hg-core/Cargo.toml
@@ -4,7 +4,6 @@
authors = ["Georges Racinet <gracinet at anybox.fr>"]
description = "Mercurial pure Rust core library, with no assumption on Python bindings (FFI)"
edition = "2018"
-build = "build.rs"
[lib]
name = "hg"
@@ -13,7 +12,6 @@
byteorder = "1.3.4"
hex = "0.4.2"
lazy_static = "1.4.0"
-libc = { version = "0.2.66", optional = true }
memchr = "2.3.3"
rand = "0.7.3"
rand_pcg = "0.2.1"
@@ -31,10 +29,3 @@
memmap = "0.7.0"
pretty_assertions = "0.6.1"
tempfile = "3.1.0"
-
-[build-dependencies]
-cc = { version = "1.0.48", optional = true }
-
-[features]
-default = []
-with-re2 = ["cc", "libc"]
diff --git a/rust/README.rst b/rust/README.rst
--- a/rust/README.rst
+++ b/rust/README.rst
@@ -36,36 +36,6 @@
One day we may use this environment variable to switch to new experimental
binding crates like a hypothetical ``HGWITHRUSTEXT=hpy``.
-Using the fastest ``hg status``
--------------------------------
-
-The code for ``hg status`` needs to conform to ``.hgignore`` rules, which are
-all translated into regex.
-
-In the first version, for compatibility and ease of development reasons, the
-Re2 regex engine was chosen until we figured out if the ``regex`` crate had
-similar enough behavior.
-
-Now that that work has been done, the default behavior is to use the ``regex``
-crate, that provides a significant performance boost compared to the standard
-Python + C path in many commands such as ``status``, ``diff`` and ``commit``,
-
-However, the ``Re2`` path remains slightly faster for our use cases and remains
-a better option for getting the most speed out of your Mercurial.
-
-If you want to use ``Re2``, you need to install ``Re2`` following Google's
-guidelines: https://github.com/google/re2/wiki/Install.
-Then, use ``HG_RUST_FEATURES=with-re2`` and
-``HG_RE2_PATH=system|<path to your re2 install>`` when building ``hg`` to
-signal the use of Re2. Using the local path instead of the "system" RE2 links
-it statically.
-
-For example::
-
- $ HG_RUST_FEATURES=with-re2 HG_RE2_PATH=system make PURE=--rust
- $ # OR
- $ HG_RUST_FEATURES=with-re2 HG_RE2_PATH=/path/to/re2 make PURE=--rust
-
Developing Rust
===============
@@ -114,14 +84,3 @@
$ cargo +nightly fmt
This requires you to have the nightly toolchain installed.
-
-Additional features
--------------------
-
-As mentioned in the section about ``hg status``, code paths using ``re2`` are
-opt-in.
-
-For example::
-
- $ cargo check --features with-re2
-
diff --git a/rust/Cargo.lock b/rust/Cargo.lock
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -42,11 +42,6 @@
source = "registry+https://github.com/rust-lang/crates.io-index"
[[package]]
-name = "cc"
-version = "1.0.50"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-
-[[package]]
name = "cfg-if"
version = "0.1.10"
source = "registry+https://github.com/rust-lang/crates.io-index"
@@ -208,12 +203,10 @@
version = "0.1.0"
dependencies = [
"byteorder 1.3.4 (registry+https://github.com/rust-lang/crates.io-index)",
- "cc 1.0.50 (registry+https://github.com/rust-lang/crates.io-index)",
"clap 2.33.0 (registry+https://github.com/rust-lang/crates.io-index)",
"crossbeam 0.7.3 (registry+https://github.com/rust-lang/crates.io-index)",
"hex 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)",
"lazy_static 1.4.0 (registry+https://github.com/rust-lang/crates.io-index)",
- "libc 0.2.67 (registry+https://github.com/rust-lang/crates.io-index)",
"log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)",
"memchr 2.3.3 (registry+https://github.com/rust-lang/crates.io-index)",
"memmap 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -655,7 +648,6 @@
"checksum autocfg 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "f8aac770f1885fd7e387acedd76065302551364496e46b3dd00860b2f8359b9d"
"checksum bitflags 1.2.1 (registry+https://github.com/rust-lang/crates.io-index)" = "cf1de2fe8c75bc145a2f577add951f8134889b4795d47466a54a5c846d691693"
"checksum byteorder 1.3.4 (registry+https://github.com/rust-lang/crates.io-index)" = "08c48aae112d48ed9f069b33538ea9e3e90aa263cfa3d1c24309612b1f7472de"
-"checksum cc 1.0.50 (registry+https://github.com/rust-lang/crates.io-index)" = "95e28fa049fda1c330bcf9d723be7663a899c4679724b34c81e9f5a326aab8cd"
"checksum cfg-if 0.1.10 (registry+https://github.com/rust-lang/crates.io-index)" = "4785bdd1c96b2a846b2bd7cc02e86b6b3dbf14e7e53446c4f54c92a361040822"
"checksum chrono 0.4.11 (registry+https://github.com/rust-lang/crates.io-index)" = "80094f509cf8b5ae86a4966a39b3ff66cd7e2a3e594accec3743ff3fabeab5b2"
"checksum clap 2.33.0 (registry+https://github.com/rust-lang/crates.io-index)" = "5067f5bb2d80ef5d68b4c87db81601f0b75bca627bc2ef76b141d7b846a3c6d9"
diff --git a/mercurial/debugcommands.py b/mercurial/debugcommands.py
--- a/mercurial/debugcommands.py
+++ b/mercurial/debugcommands.py
@@ -1650,13 +1650,6 @@
fm.plain(_(b'checking "re2" regexp engine (%s)\n') % re2)
fm.data(re2=bool(util._re2))
- rust_debug_mod = policy.importrust("debug")
- if rust_debug_mod is not None:
- re2_rust = b'installed' if rust_debug_mod.re2_installed else b'missing'
-
- msg = b'checking "re2" regexp engine Rust bindings (%s)\n'
- fm.plain(_(msg % re2_rust))
-
# templates
p = templater.templatepaths()
fm.write(b'templatedirs', b'checking templates (%s)...\n', b' '.join(p))
To: Alphare, #hg-reviewers
Cc: mercurial-patches, mercurial-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mercurial-scm.org/pipermail/mercurial-patches/attachments/20200529/38f2196a/attachment-0001.html>
More information about the Mercurial-patches
mailing list