Patchwork D7118: rust-dirstatemap: remove additional lookups in traverse

login
register
mail settings
Submitter phabricator
Date Oct. 16, 2019, 1:48 p.m.
Message ID <differential-rev-PHID-DREV-g2tzrcjpqvl43cznhomd-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/42384/
State Superseded
Headers show

Comments

phabricator - Oct. 16, 2019, 1:48 p.m.
Alphare created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  We use the same trick as the Python implementation.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D7118

AFFECTED FILES
  mercurial/dirstate.py

CHANGE DETAILS




To: Alphare, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - Oct. 18, 2019, 10:44 a.m.
> +++ b/mercurial/dirstate.py
> @@ -919,6 +919,9 @@
>          matchalways = match.always()
>          matchtdir = match.traversedir
>          dmap = self._map
> +        if rustmod is not None:
> +            dmap = self._map._rustmap

If it's the same trick, can't it be abstracted away? `if rustmod` seems weird.
phabricator - Oct. 18, 2019, 10:47 a.m.
yuja added a comment.


  > +++ b/mercurial/dirstate.py
  > @@ -919,6 +919,9 @@
  >
  >   matchalways = match.always()
  >   matchtdir = match.traversedir
  >   dmap = self._map
  >
  > +        if rustmod is not None:
  > +            dmap = self._map._rustmap
  
  If it's the same trick, can't it be abstracted away? `if rustmod` seems weird.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7118/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7118

To: Alphare, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - Oct. 18, 2019, 12:24 p.m.
Alphare added a comment.


  In D7118#104834 <https://phab.mercurial-scm.org/D7118#104834>, @yuja wrote:
  
  >> +++ b/mercurial/dirstate.py
  >> @@ -919,6 +919,9 @@
  >>
  >>   matchalways = match.always()
  >>   matchtdir = match.traversedir
  >>   dmap = self._map
  >>
  >> +        if rustmod is not None:
  >> +            dmap = self._map._rustmap
  >
  > If it's the same trick, can't it be abstracted away? `if rustmod` seems weird.
  
  The main issue I faced is that Python classes built from `rust-cpython` cannot be subclassed, so we have to resort to an additional level of indirection. IIRC, the main reason was that there was no way of enforcing a subclass to call `super()` which would result in the Rust class not being initialized properly. Or maybe I'm missing the point?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7118/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7118

To: Alphare, #hg-reviewers
Cc: yuja, mercurial-devel
Yuya Nishihara - Oct. 18, 2019, 12:35 p.m.
>   >> +++ b/mercurial/dirstate.py
>   >> @@ -919,6 +919,9 @@
>   >>
>   >>   matchalways = match.always()
>   >>   matchtdir = match.traversedir
>   >>   dmap = self._map
>   >>
>   >> +        if rustmod is not None:
>   >> +            dmap = self._map._rustmap
>   >
>   > If it's the same trick, can't it be abstracted away? `if rustmod` seems weird.
>   
>   The main issue I faced is that Python classes built from `rust-cpython` cannot be subclassed, so we have to resort to an additional level of indirection. IIRC, the main reason was that there was no way of enforcing a subclass to call `super()` which would result in the Rust class not being initialized properly. Or maybe I'm missing the point?

I mean there could be a unified `self._map._map` call if we need to take
the internal map from both py dirstatemap and rust dirstatemap.
phabricator - Oct. 18, 2019, 12:40 p.m.
yuja added a comment.


  >   >> +++ b/mercurial/dirstate.py
  >   >> @@ -919,6 +919,9 @@
  >   >>
  >   >>   matchalways = match.always()
  >   >>   matchtdir = match.traversedir
  >   >>   dmap = self._map
  >   >>
  >   >> +        if rustmod is not None:
  >   >> +            dmap = self._map._rustmap
  >   >
  >   > If it's the same trick, can't it be abstracted away? `if rustmod` seems weird.
  >   The main issue I faced is that Python classes built from `rust-cpython` cannot be subclassed, so we have to resort to an additional level of indirection. IIRC, the main reason was that there was no way of enforcing a subclass to call `super()` which would result in the Rust class not being initialized properly. Or maybe I'm missing the point?
  
  I mean there could be a unified `self._map._map` call if we need to take
  the internal map from both py dirstatemap and rust dirstatemap.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7118/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7118

To: Alphare, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - Dec. 10, 2019, 6:30 p.m.
martinvonz added a comment.


  What's the state of this patch? The description makes it sound like it's a Rust patch, but it only modifies Python code.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7118/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7118

To: Alphare, #hg-reviewers
Cc: martinvonz, yuja, mercurial-devel
phabricator - Dec. 11, 2019, 8:37 a.m.
Alphare added a comment.


  In D7118#111735 <https://phab.mercurial-scm.org/D7118#111735>, @martinvonz wrote:
  
  > What's the state of this patch? The description makes it sound like it's a Rust patch, but it only modifies Python code.
  
  It only impacts the rust module policy, I figured the name was not a bad idea. Maybe I'm wrong?
  
  I also did not find Yuya's suggestion to work, but I may have understood it wrong.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7118/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7118

To: Alphare, #hg-reviewers
Cc: martinvonz, yuja, mercurial-devel
phabricator - Dec. 12, 2019, 4:46 p.m.
martinvonz added a comment.


  In D7118#111784 <https://phab.mercurial-scm.org/D7118#111784>, @Alphare wrote:
  
  > In D7118#111735 <https://phab.mercurial-scm.org/D7118#111735>, @martinvonz wrote:
  >
  >> What's the state of this patch? The description makes it sound like it's a Rust patch, but it only modifies Python code.
  >
  > It only impacts the rust module policy, I figured the name was not a bad idea. Maybe I'm wrong?
  > I also did not find Yuya's suggestion to work, but I may have understood it wrong.
  
  I think Yuya's suggestion was to make the diff be this:
  
    diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
    --- a/mercurial/dirstate.py
    +++ b/mercurial/dirstate.py
    @@ -909,7 +909,7 @@ class dirstate(object):
             matchfn = match.matchfn
             matchalways = match.always()
             matchtdir = match.traversedir
    -        dmap = self._map
    +        dmap = self._map._map
             listdir = util.listdir
             lstat = os.lstat
             dirkind = stat.S_IFDIR
  
  Obviously, you'd need to make some adjustments to allow that, at least renaming `_rustmap` to `_map`. The commit message should also change then.
  
  I don't remember what the point of the nested `_map`s was (perhaps to make it possible to replace the inner `_map`?). Hopefully we're not breaking that by directly accessing the inner `_map`.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7118/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7118

To: Alphare, #hg-reviewers
Cc: martinvonz, yuja, mercurial-devel

Patch

diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -919,6 +919,9 @@ 
         matchalways = match.always()
         matchtdir = match.traversedir
         dmap = self._map
+        if rustmod is not None:
+            dmap = self._map._rustmap
+
         listdir = util.listdir
         lstat = os.lstat
         dirkind = stat.S_IFDIR