Skip to content

Conversation

petekanev
Copy link
Contributor

Changed object instantiation to happen in the C++ code instead of Java in order to normalize constructor/method invocations.

Like methods, constructors are resolved in Java and their signatures cached in C++, where we make instances of Java objects. This means that arguments will be converted the same way for both methods and constructors.

ping: @Plamen5kov, @slavchev

…ray of jvalue

add working object instantiation in cpp

refactor unused and unaccessed code

add constructor caching with the updated structure
@ns-bot
Copy link

ns-bot commented Apr 12, 2016

💚

@petekanev petekanev changed the title Pete/unify method constructor pipeline Change and normalize object instantiation in C++ Apr 12, 2016
@petekanev
Copy link
Contributor Author

Some benchmarking on the performance - An object is created 100 000 times, constructors are cached after the first invocation:

(debug config - please ignore)

Refactored Code Old Code
Creating object with default ctor - ~~~2030ms~~ Creating object with default ctor - ~~~1800ms~~
Creating object with one arg - ~~~2400ms~~ Creating object with one arg - ~~~2115ms~~
Creating object with two args - ~~~2900ms~~ Creating object with two arg - ~~~2820ms~~

*both args passed are reference types

@atanasovg
Copy link
Contributor

@Pip3r4o So, the benchmarks show that we are actually getting slower with this pull? What's the benefit then? The Runtimes should be extra cautious about performance, especially in Android. Btw, I wonder why is this slower? Moving the logic to C++ implies that it would get faster. Could it be that still some work is done in Java and it is the marshaling transitions that make the difference?

@petekanev
Copy link
Contributor Author

@atanasovg I am still trying to pinpoint the exact reason(s) for the discrepancies. For one, I know that once all candidate constructors are found in Java, we do one additional conversion (again in Java) to confirm arguments match with the best candidate, which could be dropped out, but I have yet to test whether there'd be side effects to that.

Previously arguments were being converted only before resolving constructor in Java land, and one more time as confirmation, in Java. I'll see if dropping either of the two additional arg conversions will make a difference.

@petekanev
Copy link
Contributor Author

@atanasovg I'd like to add that the above tests were performed in debug configuration, which I hadn't noticed earlier.

I am happy to say that with the removal of the redundant conversions, the latest benchmarks show that the changes account for slightly better performance than what we had before, and perform as fast as the previous version when there are one or more arguments.

Refactored Code Old Code
Creating object with default ctor - ~790ms Creating object with default ctor - ~910ms
Creating object with one arg - ~950ms Creating object with one arg - ~950ms
Creating object with two args - ~1150ms Creating object with two arg - ~1250ms

@atanasovg
Copy link
Contributor

👍 @Pip3r4o Me likes this 😄

@slavchev
Copy link

👍
I expect that this refactoring should be in general faster than the Java implementation and indeed your benchmarks confirm it. I see more opportunities here. If you use shared memory you can remove the two SetStaticIntField invocations and get even better performance. Another argument for doing this is to eliminate potential issues when an exception is thrown from within the constructor. Using RAII is the proper solution for this. Good job.

@ns-bot
Copy link

ns-bot commented Apr 13, 2016

💚

@Plamen5kov
Copy link
Contributor

@Pip3r4o, looks great 👍

@ns-bot
Copy link

ns-bot commented Apr 13, 2016

💚

@petekanev petekanev merged commit 64ceb38 into master Apr 13, 2016
@petekanev petekanev deleted the pete/unify-method-constructor-pipeline branch April 19, 2016 12:05
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.

5 participants