You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Niraj Tolia <no...@github.com> on 2013/10/01 10:53:04 UTC
[jclouds] [RFC] Make increased use of Invokable params cache (#164)
While this patch probably violates good design principles because of
the imperfect location of the cache, exposing the params cache outside
of RestAnnotationProcessor to S3Utils results in a roughly 40%
improvement in performance for generating signed GETs and PUTs for
S3. This commit also converts a few others calls to
Invokable.getParameters() but the observed benefit from those was
small in my microbenchmarks.
This is related to JCLOUDS 301.
You can merge this Pull Request by running:
git pull https://github.com/maginatics/jclouds params_cache
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/164
-- Commit Summary --
* [RFC]: Make increased use of Invokable params cache
-- File Changes --
M apis/s3/src/main/java/org/jclouds/s3/util/S3Utils.java (3)
M core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java (10)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/164.patch
https://github.com/jclouds/jclouds/pull/164.diff
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Andrew Phillips <no...@github.com>.
> @@ -77,7 +78,7 @@ public static String getBucketName(HttpRequest req) {
>
> String bucketName = null;
>
> - ImmutableList<Parameter> parameters = request.getInvocation().getInvokable().getParameters();
> + List<Parameter> parameters = Reflection2.getInvokableParameters(request.getInvocation().getInvokable());
Sounds good - `List<...>` it is, then!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164/files#r6723934
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #271](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/271/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25601148
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Adrian Cole <no...@github.com>.
I'm not bothered about sharing a cache, more limiting further exposure of
RestAnnotationProcessor. If you have an approach that satisfies that, the
go for it!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25468132
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #742](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/742/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25850632
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Niraj Tolia <no...@github.com>.
Spurious failure? I cannot repro using ```mvn clean test``` locally.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25584333
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #732](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/732/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25601156
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Adrian Cole <no...@github.com>.
Good idea, addressing this.
I think it would be better to duplicate the cache than expose more of RestAnnotationProcessor's internals.
We are only talking about a few methods, so the cache wont be big.
make sense?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25461468
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Andrew Phillips <no...@github.com>.
> @@ -776,7 +770,7 @@ private static void addHeader(Multimap<String, String> headers, Headers header,
> }
>
> private boolean checkPresentOrNullable(Invocation invocation, String paramKey, int argIndex, Object arg) {
> - if (arg == null && !invocation.getInvokable().getParameters().get(argIndex).isAnnotationPresent(Nullable.class))
> + if (arg == null && !Reflection2.getInvokableParameters(invocation.getInvokable()).get(argIndex).isAnnotationPresent(Nullable.class))
Static import of `Reflection2.getInvokableParameters`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164/files#r6710179
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Ignasi Barrera <no...@github.com>.
If I am not wrong, being static implicitly shares the cache between contexts. Is there any scenario where this could be a problem? (For example different providers using the same common api). If that is the case, should we cover it?
Don't want to stop this PR (lgtm too), just asking to make sure there aren't unknown side effects.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25599903
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Andrew Gaul <no...@github.com>.
Tested against AWS-S3. Pushed to master and 1.6.x.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-26301582
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #262](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/262/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25479213
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Adrian Cole <no...@github.com>.
Since both objects that use this cache are wired by guice, we could avoid
both exposing this cache public and also avoid dealing with any side
effects of statically maintained state by using guice instead.
There are many other caches in jclouds that are wired via guice.
Personally, I'd prefer to not have to even consider classloader (OSGi) or
other side effects of shared static state and either maintain the cache
local to the 2 users or shared via guice.
OTOH, I don't want to be overly prescriptive, so.. above in mind, make a
call :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25627721
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Andrew Phillips <no...@github.com>.
> Spurious failure? I cannot repro using mvn clean test locally.
I'm guessing so, but since it doesn't seem an obvious timing-related [failure](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/org.apache.jclouds$jclouds-compute/466/testReport/junit/org.jclouds.compute.callables/BlockUntilInitScriptStatusIsZeroThenReturnOutputTest/testloopUntilTrueOrThrowCancellationExceptionReturnsWhenPredicateIsTrueSecondTimeWhileNotCancelled/), I'll close-n-reopen this request to run BuildHive etc. again.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25600500
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Andrew Gaul <no...@github.com>.
Sorry, I commented and closed the wrong pull request. I will look at this soon.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25849321
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Andrew Phillips <no...@github.com>.
> Looks like an timing-related test failure
Thanks for checking this out, @ntolia - I think we can indeed ignore this test failure ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-26034288
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #730](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/730/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25568070
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Andrew Phillips <no...@github.com>.
> @@ -195,6 +195,24 @@ public boolean apply(Invokable<?, ?> input) {
> }
> });
>
> + private static final LoadingCache<Invokable<?, ?>, ImmutableList<Parameter>> invokableParamsCache =
> + CacheBuilder.newBuilder().maximumSize(100).build(new CacheLoader<Invokable<?, ?>, ImmutableList<Parameter>>() {
Indenting? jclouds uses 3 spaces...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164/files#r6710128
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Andrew Phillips <no...@github.com>.
> @@ -77,7 +78,7 @@ public static String getBucketName(HttpRequest req) {
>
> String bucketName = null;
>
> - ImmutableList<Parameter> parameters = request.getInvocation().getInvokable().getParameters();
> + List<Parameter> parameters = Reflection2.getInvokableParameters(request.getInvocation().getInvokable());
I'm fine with the type change, but looking at the implementation this could also remain `ImmutableList`. Is there any specific reason for the change?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164/files#r6710165
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Niraj Tolia <no...@github.com>.
Ideally, I would like to avoid the reflection overhead altogether instead of paying it twice here. Would moving the Cache out of RestAnnotationProcessor into something else be acceptable? Not sure of the right home but Reflections2.java does leap out at me just based on the name. If you would prefer duplicating Caches, let me run numbers with a single Cache and with a similarly sized duplicated Cache and measure the performance difference. Hopefully the data will be enough to convince us of one approach or the other.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25467696
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Andrew Phillips <no...@github.com>.
Commits:
* [master](https://git-wip-us.apache.org/repos/asf?p=incubator-jclouds.git;a=commit;h=53146fb7bb746309b5e1c4abe0dd12e6d9f0d008)
* [1.6.x](https://git-wip-us.apache.org/repos/asf?p=incubator-jclouds.git;a=commit;h=6b8ee8b904ae831b04d6f9355ee061e452408ad7)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-26334336
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Niraj Tolia <no...@github.com>.
> @@ -776,7 +770,7 @@ private static void addHeader(Multimap<String, String> headers, Headers header,
> }
>
> private boolean checkPresentOrNullable(Invocation invocation, String paramKey, int argIndex, Object arg) {
> - if (arg == null && !invocation.getInvokable().getParameters().get(argIndex).isAnnotationPresent(Nullable.class))
> + if (arg == null && !Reflection2.getInvokableParameters(invocation.getInvokable()).get(argIndex).isAnnotationPresent(Nullable.class))
Done in this class.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164/files#r6723355
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Niraj Tolia <no...@github.com>.
> @@ -195,6 +195,24 @@ public boolean apply(Invokable<?, ?> input) {
> }
> });
>
> + private static final LoadingCache<Invokable<?, ?>, ImmutableList<Parameter>> invokableParamsCache =
> + CacheBuilder.newBuilder().maximumSize(100).build(new CacheLoader<Invokable<?, ?>, ImmutableList<Parameter>>() {
> + @Override
> + public ImmutableList<Parameter> load(Invokable<?, ?> invokable) {
> + return invokable.getParameters();
> + }
> + });
> +
> + /**
> + * Returns the {@link Parameter}s associated with the given {@link Invokable}. This function is back by a cache.
Fixed the typo. I left the ```@return``` comment out for consistency as it seems like none of the other functions in this class have them. LMK if you prefer it as I would be happy to move things around.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164/files#r6723352
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Andrew Phillips <no...@github.com>.
> @@ -195,6 +195,24 @@ public boolean apply(Invokable<?, ?> input) {
> }
> });
>
> + private static final LoadingCache<Invokable<?, ?>, ImmutableList<Parameter>> invokableParamsCache =
> + CacheBuilder.newBuilder().maximumSize(100).build(new CacheLoader<Invokable<?, ?>, ImmutableList<Parameter>>() {
> + @Override
> + public ImmutableList<Parameter> load(Invokable<?, ?> invokable) {
> + return invokable.getParameters();
> + }
> + });
> +
> + /**
> + * Returns the {@link Parameter}s associated with the given {@link Invokable}. This function is back by a cache.
"is backed"
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164/files#r6710134
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Niraj Tolia <no...@github.com>.
@nacx Great question and I did think about this. I believe that as long as the Invokable's equals() method does the right thing, there should be no conflict. Now, Invokable.equals() is Element.equals() which invokes Object.equals() (via Member) and that is simply a memory equality check (```==```). I therefore think we should be OK. Would be great if someone like @adriancole can confirm my understanding of the code too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25600303
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Andrew Phillips <no...@github.com>.
+1 - looks good to me. Just waiting for BuildHive et al now...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25564435
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Adrian Cole <no...@github.com>.
Sounds like a reasonable call.
Go for it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25640358
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Niraj Tolia <no...@github.com>.
@adriancole There is no public cache in the most recent patch and the cache is hidden behind Reflection2.invokableParamsCache() and therefore external visibility should not be an issue. Note that there are 8 existing ```private static``` LoadingCaches defined in Reflection2.java that aren't bound by Guice either that are shared across all callers. Assuming this pattern that worked so far, my patch just follows that. Does that sound good?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25639614
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #466](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/466/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25568590
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #261](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/261/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25435125
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #723](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/723/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25479226
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #479](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/479/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25852017
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Niraj Tolia <no...@github.com>.
> @@ -195,6 +195,24 @@ public boolean apply(Invokable<?, ?> input) {
> }
> });
>
> + private static final LoadingCache<Invokable<?, ?>, ImmutableList<Parameter>> invokableParamsCache =
> + CacheBuilder.newBuilder().maximumSize(100).build(new CacheLoader<Invokable<?, ?>, ImmutableList<Parameter>>() {
Done
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164/files#r6723296
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #722](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/722/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25435137
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Niraj Tolia <no...@github.com>.
> @@ -77,7 +78,7 @@ public static String getBucketName(HttpRequest req) {
>
> String bucketName = null;
>
> - ImmutableList<Parameter> parameters = request.getInvocation().getInvokable().getParameters();
> + List<Parameter> parameters = Reflection2.getInvokableParameters(request.getInvocation().getInvokable());
This was something I introduced in a previous change but I shouldn't have. There is nothing in the below code that uses any ImmutableList specific functions and I generally prefer using the least specific type if possible. This is also reflected in a couple of the other changes below.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164/files#r6723292
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #280](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/280/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25850617
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Adrian Cole <no...@github.com>.
Sounds good. Less complex than sharing via guice, which would be the alternative.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25498982
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Niraj Tolia <no...@github.com>.
OK, I pulled the cache out of RestAnnotationProcessor and into Reflection2. While refactoring, I did run a test with a split cache and it does show noticeable benefits but this approach has slightly bigger wins and the move into Reflection2 seems cleaner to me.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25481469
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by Niraj Tolia <no...@github.com>.
Looks like an timing-related test failure - org.jclouds.rest.functions.PresentWhenApiVersionLexicographicallyAtOrAfterSinceApiVersionTest.testCacheIsFasterWhenAnnotationPresent. Also, note that there were no changes when compared to the last successful build but this was simply an accidental open/close of this pull request.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-26029943
Re: [jclouds] [RFC] Make increased use of Invokable params cache
(#164)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #269](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/269/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/164#issuecomment-25564745