Patchwork [1,of,3] rust: module policy with importrust

login
register
mail settings
Submitter Georges Racinet
Date May 23, 2019, 11:29 a.m.
Message ID <36b4c4869531b3de0b10.1558610945@purity.tombe.racinet.fr>
Download mbox | patch
Permalink /patch/40207/
State Superseded
Headers show

Comments

Georges Racinet - May 23, 2019, 11:29 a.m.
# HG changeset patch
# User Georges Racinet <georges.racinet@octobus.net>
# Date 1558603154 -7200
#      Thu May 23 11:19:14 2019 +0200
# Node ID 36b4c4869531b3de0b101c941b77b94da85890b7
# Parent  52beb1b8a64967bead7bc9e8a63c460430729b7a
# EXP-Topic rust-modulepolicy
rust: module policy with importrust

We introduce two rust+c module policies and a new
`policy.importrust()` that makes use of them.

This simple approach provides runtime switching of
implementations, which is crucial for the performance
measurements such as those Octobus does with ASV.

It can also be useful for bug analysis.

It also has the advantage of making conditionals in
Rust callers more uniform, in particular
abstracting over specifics like `demandimport`

At this point, the build stays unchanged, with the rust-cpython based
`rustext` module being built if HGWITHRUSTEXT=cpython.

More transparency for the callers, i.e., just using
`policy.importmod` would be a much longer term and riskier
effort for the following reasons:

1. It would require to define common module boundaries
   for the three or four cases (pure, c, rust+ext, cffi) and that
   is premature with the Rust extension currently under heavy
   development in areas that are outside the scope of the C extensions.
2. It would imply internal API changes that are not currently wished,
   as the case of ancestors demonstrates.
3. The lack of data or property-like attributes (tp_member
   and tp_getset) in current `rust-cpython` makes it impossible to
   achieve direct transparent replacement of pure Python classes by
   Rust extension code, meaning that the caller sometimes has to be able
   to make adjustments or provide additional wrapping.
Yuya Nishihara - May 23, 2019, 11:35 p.m.
On Thu, 23 May 2019 13:29:05 +0200, Georges Racinet wrote:
> # HG changeset patch
> # User Georges Racinet <georges.racinet@octobus.net>
> # Date 1558603154 -7200
> #      Thu May 23 11:19:14 2019 +0200
> # Node ID 36b4c4869531b3de0b101c941b77b94da85890b7
> # Parent  52beb1b8a64967bead7bc9e8a63c460430729b7a
> # EXP-Topic rust-modulepolicy
> rust: module policy with importrust

(not reviewed thoroughly)

> +def importrust(modname):
> +    """Import Rust module according to policy and availability.
> +
> +    If policy is non-Rust, this returns None
> +    If policy is Rust and the module is non available, returns None if policy
> +    is '-allow' else raise ImportError
> +    """
> +    if not policy.startswith('rust'):

Perhaps, it's b''.

> +        return None
> +    try:
> +        mod = _importfrom('rustext', modname)
> +    except ImportError:
> +        if policy.endswith('-allow'):
> +            return None
> +        raise
> +    return mod

How about making it return a cext module (or provided default) as a fallback?

  mayberustext = policy.importrust(r'parsers')
  # or
  mayberustext = policy.importrust('parers', parsers)

I think some "if rust is not None" can be removed with this trick, and
eventually the importrust() can be merged with the importmod().
Georges Racinet - May 24, 2019, 10:06 a.m.
On 5/24/19 1:35 AM, Yuya Nishihara wrote:
> On Thu, 23 May 2019 13:29:05 +0200, Georges Racinet wrote:
>> # HG changeset patch
>> # User Georges Racinet <georges.racinet@octobus.net>
>> # Date 1558603154 -7200
>> #      Thu May 23 11:19:14 2019 +0200
>> # Node ID 36b4c4869531b3de0b101c941b77b94da85890b7
>> # Parent  52beb1b8a64967bead7bc9e8a63c460430729b7a
>> # EXP-Topic rust-modulepolicy
>> rust: module policy with importrust
> (not reviewed thoroughly)
Thanks anyway for looking at it
>
>> +def importrust(modname):
>> +    """Import Rust module according to policy and availability.
>> +
>> +    If policy is non-Rust, this returns None
>> +    If policy is Rust and the module is non available, returns None if policy
>> +    is '-allow' else raise ImportError
>> +    """
>> +    if not policy.startswith('rust'):
> Perhaps, it's b''.
Ah yes, of course, will have to acquire that reflex.
>> +        return None
>> +    try:
>> +        mod = _importfrom('rustext', modname)
>> +    except ImportError:
>> +        if policy.endswith('-allow'):
>> +            return None
>> +        raise
>> +    return mod
> How about making it return a cext module (or provided default) as a fallback?
>
>   mayberustext = policy.importrust(r'parsers')
>   # or
>   mayberustext = policy.importrust('parers', parsers)
>
> I think some "if rust is not None" can be removed with this trick, and
> eventually the importrust() can be merged with the importmod().

In dirstate.py, we'd have something like

    maybenative = importrust(r'dirstate', default=parsers)

and indeed, later on, no need to check for None. That's interesting.

Semantically, there's a risk the users of that API might not realize
what this defaulting really means. We'd still want a hard error if the
policy isn't 'rust+c-allow', because that's actually protection against
accidentally wrong performance measurement or conclusions in case of bug
investigation, but I'd make that very clear in the docstring.

Perhaps a more complete form would be

    def importrust(modname, member=None, default=None):

This would be usable almost directly in my current series for discovery:

    class pypartialdiscovery:  # rename of current partialdiscovery class

    (...)

    partialdiscovery = importrust(r'discovery',
member=r'PartialDiscovery', default=pypartialdiscovery)

That way, we don't need to push the partialdiscovery class in a separate
module, nor do we need to have identical naming conventions in the Rust
and Python codes (we discussed this some months ago in the context of
ancestors).

Overall, this would give us more options to reduce the pollution in the
code base by gradually making the language variants more uniform.

Patch

diff -r 52beb1b8a649 -r 36b4c4869531 mercurial/debugcommands.py
--- a/mercurial/debugcommands.py	Wed May 22 14:16:44 2019 -0700
+++ b/mercurial/debugcommands.py	Thu May 23 11:19:14 2019 +0200
@@ -1278,16 +1278,28 @@ 
     fm.write('hgmodules', _("checking installed modules (%s)...\n"),
              os.path.dirname(pycompat.fsencode(__file__)))
 
-    if policy.policy in ('c', 'allow'):
+    rustandc = policy.policy in ('rust+c', 'rust+c-allow')
+    rustext = rustandc  # for now, that's the only case
+    cext = policy.policy in ('c', 'allow') or rustandc
+    nopure = cext or rustext
+    if nopure:
         err = None
         try:
-            from .cext import (
-                base85,
-                bdiff,
-                mpatch,
-                osutil,
-            )
-            dir(bdiff), dir(mpatch), dir(base85), dir(osutil) # quiet pyflakes
+            if cext:
+                from .cext import (
+                    base85,
+                    bdiff,
+                    mpatch,
+                    osutil,
+                )
+                # quiet pyflakes
+                dir(bdiff), dir(mpatch), dir(base85), dir(osutil)
+            if rustext:
+                from .rustext import (
+                    ancestor,
+                    dirstate,
+                )
+                dir(ancestor), dir(dirstate) # quiet pyflakes
         except Exception as inst:
             err = stringutil.forcebytestr(inst)
             problems += 1
diff -r 52beb1b8a649 -r 36b4c4869531 mercurial/policy.py
--- a/mercurial/policy.py	Wed May 22 14:16:44 2019 -0700
+++ b/mercurial/policy.py	Thu May 23 11:19:14 2019 +0200
@@ -13,6 +13,9 @@ 
 # Rules for how modules can be loaded. Values are:
 #
 #    c - require C extensions
+#    rust+c - require Rust and C extensions
+#    rust+c-allow - allow Rust and C extensions with fallback to pure Python
+#                   for each
 #    allow - allow pure Python implementation when C loading fails
 #    cffi - required cffi versions (implemented within pure module)
 #    cffi-allow - allow pure Python implementation if cffi version is missing
@@ -29,6 +32,9 @@ 
     b'cffi': (r'cffi', None),
     b'cffi-allow': (r'cffi', r'pure'),
     b'py': (None, r'pure'),
+    # For now, rust policies impact importrust only
+    b'rust+c': (r'cext', None),
+    b'rust+c-allow': (r'cext', r'pure'),
 }
 
 try:
@@ -107,3 +113,20 @@ 
                 raise
     pn, mn = _modredirects.get((purepkg, modname), (purepkg, modname))
     return _importfrom(pn, mn)
+
+def importrust(modname):
+    """Import Rust module according to policy and availability.
+
+    If policy is non-Rust, this returns None
+    If policy is Rust and the module is non available, returns None if policy
+    is '-allow' else raise ImportError
+    """
+    if not policy.startswith('rust'):
+        return None
+    try:
+        mod = _importfrom('rustext', modname)
+    except ImportError:
+        if policy.endswith('-allow'):
+            return None
+        raise
+    return mod
diff -r 52beb1b8a649 -r 36b4c4869531 setup.py
--- a/setup.py	Wed May 22 14:16:44 2019 -0700
+++ b/setup.py	Thu May 23 11:19:14 2019 +0200
@@ -556,10 +556,17 @@ 
         if self.distribution.pure:
             modulepolicy = 'py'
         elif self.build_lib == '.':
-            # in-place build should run without rebuilding C extensions
-            modulepolicy = 'allow'
+            # in-place build should run without rebuilding C
+            # and Rust extensions
+            if hgrustext == 'cpython':
+                modulepolicy = 'rust+c-allow'
+            else:
+                modulepolicy = 'allow'
         else:
-            modulepolicy = 'c'
+            if hgrustext == 'cpython':
+                modulepolicy = 'rust+c'
+            else:
+                modulepolicy = 'c'
 
         content = b''.join([
             b'# this file is autogenerated by setup.py\n',