Hi David,
Thank you for sharing your opinion!
I'm taking of my claim then. :)
On 12/5/16 22:07, David Holmes wrote:
Hi Serguei,
On 6/12/2016 3:57 PM, serguei.spit...@oracle.com wrote:
Hi Dmitry,
I'm repeating my comments from the internal review.
It looks pretty good to me.
Thank you for the work on it!
I'm not a big fun of new jvmtiThreadState functions though:
+ inline void save_exception_state(ExceptionState *state) { *state =
_exception_state; }
This saves _exception_state into *state.
+ inline void restore_exception_state(ExceptionState state) {
_exception_state = state; }
This restores _exception_state from state.
The functions above do not really
encapsulate and save/restore the _exception_state value so that these
I think they do.
Just to explain.
Of course, I see the prefixes save_ and restore_. :)
I was concerned about the direction of saving/restoring.
The consumer of the api is some other class, not the class itself.
In this case, there is no encapsulation of the saved/restored value as
the value is stored away of the object that should manage it.
Then a misapplication is possible: one value is saved - another restored.
The examples I like are:
interpreter/templateInterpreterGenerator.hpp: void
restore_native_result(void);
interpreter/templateInterpreterGenerator.hpp: void
save_native_result(void);
runtime/sharedRuntime.hpp: static void
restore_native_result(MacroAssembler *_masm, BasicType ret_type, int
frame_slots);
runtime/sharedRuntime.hpp: static void
save_native_result(MacroAssembler *_masm, BasicType ret_type, int
frame_slots);
services/threadService.hpp: save_old_state(java_thread);
opto/superword.hpp: void store_depth() {_depth_save = _depth;}
opto/superword.hpp: void restore_depth() {_depth = _depth_save;}
However, it looks minor to me, maybe a matter of taste.
I'm silent now.
Thanks!
Serguei
Cheers,
David
-----
names are kind of misleading. The same functionality can be provided
with a simplified approach: inline JvmtiThreadState
exception_state() { return_exception_state; }
inline void set_exception_state(ExceptionState state){
_exception_state = state; }
I do not want to insist on my approach and will wait for other people
comments on this.
I hope, some compromise can be found here.
On 12/5/16 01:30, Dmitry Samersoff wrote:
Everybody,
Please review.
http://cr.openjdk.java.net/~dsamersoff/JDK-8165496/webrev.07/
Bug link:
https://bugs.openjdk.java.net/browse/JDK-8165496
Two separate flags, exception_detected and exception_caught, replaced
with one that changes it's state.
Assert assert(_exception_caught == false) and fix for JDK-8134434 are
removed.
Testing:
1. Manual testing of instrumented hotspot
2. Local ks with stress agent
3. RBT ks with stress agent
What the 'ks' stands for?
I have some guess but still would like to know this abbreviation.
Thanks,
Serguei
4. RBT hotspot_all
-Dmitry
