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

Cache compunits by id instead of spec str #1121

Closed
wants to merge 1 commit into from
Closed

Cache compunits by id instead of spec str #1121

wants to merge 1 commit into from

Conversation

ugexe
Copy link
Member

@ugexe ugexe commented Jul 22, 2017

Previously this would cache on a spec string, but the spec
used was the query and not a concrete spec.

The earliest a concrete identity can be obtained to cache is
after calling !matching-dist, which handles all the range stuff
so we don't cache something like "6.*" when "6.c" is what will be
loaded. While we could also continue to use the string identity
(just from this concrete version) that would require a Distribution
to be created that is othewise saved by caching on the file-id.

Previously this would cache on a spec string, but the spec
used was the *query* and not a concrete spec.

The earliest a concrete identity can be obtained to cache is
after calling !matching-dist, which handles all the range stuff
so we don't cache something like "6.*" when "6.c" is what will be
loaded. While we could also continue to use the string identity
(just from this concrete version) that would require a Distribution
to be created that is othewise saved by caching on the file-id.
@ugexe ugexe requested a review from niner July 22, 2017 19:37
@niner
Copy link
Collaborator

niner commented Jul 23, 2017

I think we should do both: cache on spec str (because it's massively cheaper) and cache on compunit id (because it will handle more cases). The vast majority of DependencySpecifications consist just of the name without any more details. Thus the chances for a cache hit based on the given specification alone are quite high. Removing the cache on spec str would make us do costly file system access just to discover, that we have already loaded the module. But yes, there's still something to be gained by caching on the compunit id.

@ugexe
Copy link
Member Author

ugexe commented Jul 23, 2017

I considered performance, but could not demonstrate any difference in the majority case:
perl6 -e 'for ^100000000 { use Test; }; say time - INIT time'
i.e. other mechanisms already take care of the majority case

@ugexe
Copy link
Member Author

ugexe commented Jul 26, 2017

I'm going to merge this tomorrow if there are no objections since it improves the current behavior without negatively impacting performance. I have addressed the suggestion of caching the spec string in Centralize CURI search logic #1120 in the commit here using code similar to that shown below (but adapted to the PR mechanics of returning a list).

Otherwise I'll have to commit this logic slightly differently for both PRs ✌️

 class CompUnit::Repository::Installation does CompUnit::Repository::Locally does CompUnit::Repository::Installable {
     has $!cver = nqp::hllize(nqp::atkey(nqp::gethllsym('perl6', '$COMPILER_CONFIG'), 'version'));
     has %!loaded;
+    has %!seen;
     has $!precomp;
     has $!id;
     has Int $!version;
@@ -418,6 +419,10 @@ sub MAIN(:$name is copy, :$auth, :$ver, *@, *%) {
     }
 
     method !matching-dist(CompUnit::DependencySpecification $spec) {
+        $!lock.protect: {
+            return %!seen{~$spec} if %!seen{~$spec}:exists;
+        }
+
         if $spec.from eq 'Perl6' {
             my $repo-version = self!repository-version;
             my $lookup = $.prefix.add('short').add(nqp::sha1($spec.short-name));
@@ -444,7 +449,9 @@ sub MAIN(:$name is copy, :$auth, :$ver, *@, *%) {
                             !! Version.new($spec.version-matcher))
                     });
                 for @dists.sort(*.value<ver>).reverse.map(*.kv) -> ($dist-id, $dist) {
-                    return ($dist-id, $dist);
+                    $!lock.protect: {
+                        return %!seen{~$spec} = ($dist-id, $dist);
+                    }
                 }
             }
         }

@ugexe
Copy link
Member Author

ugexe commented Jul 30, 2017

Superceded by #1125

@ugexe ugexe closed this Jul 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants