-
Notifications
You must be signed in to change notification settings - Fork 694
Fixed incorrect docs PersistentPropertyPath and optimized sublist #2782
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
Fixed incorrect docs PersistentPropertyPath and optimized sublist #2782
Conversation
Mixing up different things in a single PR makes reviewing them pretty cumbersome because we might decide we like one change but not the other. Can you elaborate what you mean by "optimize"? What exactly? For whom? You also introduce new API, without a clear reason why that's needed.
Let me explain then one by one
- Optimization -
getExtensionForBaseOf
method previously usedisBasePathOf
. This method iterated over the the properties path and checks whenever the current path is base for passed. And then we just reiterate over the whole properties, when we can just put the remaining part. You can check the code yourself, it is just skipping the first N elements and only then adds the remaining - we can just do the sublist. We do not spend time now to reiterate on the part we have already checked - Optimization for whom - for any spring-data project, for spring-data-jdbc in particular.
- Introduction of the new API is arguable, I agree - I can make method private - I though it was reasonable to abstract the logic from
isBasePathOf
. What it dose - it actually does 2 things - it tries to find the common part and then ensures that the common part is the same asthis
. I though we can separate this 2 things, and I decided to make it public, maybe it will be needed by projects. I can remove it, if you think it make sense, but in general methodisBasePathOf
does 2 things
Thanks for the detailed explanation. Have you run any JMH benchmarks to measure the difference in performance and memory consumption. While I totally see that we can just use ....subList(...)
in getExtensionForBaseOf(...)
, I'm not sure whether the change in getCommonBaseWith(...)
really improves the situation. Your suggestion creates both a temporary List
, a reference DefaultPersistentPropertyPath
from that for the latter to be piped into an equals(...)
comparison which will in turn again have to traverse both internal collections. Superficially, that doesn't sound more efficient to me that just traversing an iterator (which your variant has to as well) and returning the result immediately.
About JMH bnchmarks - I will run against the getExtensionForBaseOf
method, thanks for suggestion.
I am not trying to say that getCommonBaseWith(...)
is anyhow better :) I have created it just to separate the responsibility - the actual check logic (condition itself) and search for the common base.
Regarding getExtensionForBaseOf
- it is supposed to be faster I wager. After making sure that passed path
is the base for current, there was such logic, which again traverses the proeprties.
List<P> result = new ArrayList<>();
Iterator<P> iterator = iterator();
for (int i = 0; i < base.getLength(); i++) {
iterator.next();
}
while (iterator.hasNext()) {
result.add(iterator.next());
}
I have just changed it to subList()
, as it will only create an instance of SubList
(in the example above there still would be ArrayList
in its place) and will not traverse the entire properties
obejct again.
But in any case, I perfectly agree that we should run JMH benchmark. Unfortunately I have not found infrastructure for this in spring-data-commons
in general. Am I supposed to just test it on my machine without pushing anything to git?
We keep a loose collection of benchmarks here. Would be nice if you just some in a PR over there so that we can run those against the current codebase and your suggestions to see what difference it makes. Note that right now, I'd only be interested in the performance of getExtensionForBaseOf(...)
and isBasePathOf(...)
are these are the only methods currently in use in Spring Data modules.
1cc7616
to
c197dba
Compare
c197dba
to
d9a14b0
Compare
Hello @odrotbohm !
I have written a GitHub repository to benchmark the changes, see here: https://github.com/Mikhail2048/spring-data-commons-benchmark-2782
It would be great if you will try to benchmark on your hardware on your side. It is pretty simple, you can do mvn clean test
and the benchmarks will be executed. Also, make sure to change the version of spring-data-commons in the pom: https://github.com/Mikhail2048/spring-data-commons-benchmark-2782/blob/master/pom.xml#L22
I have current version build and I switched to my branch, build spring-data-commons locally from my branch put the version into the pom.xml
.
I have received the following results.
This is the current state:
image
This is my changes:
image
As you can see, there is almost no difference. So, take a look at PR again please, and let's decide what to do. I think I can just leave the documentation changes.
Thanks in advance!
fe5e80e
to
98f20a4
Compare
0122d1b
to
5070a0f
Compare
2c56532
to
b2c3623
Compare
646e077
to
6ac5cf8
Compare
1f86a58
to
3a61461
Compare
Fixed incorrect docs in
PersistentPropertyPath#isBasePathOf
and optimizedgetExtensionForBaseOf
sublisting