I'd like to re-boot this review. I've been working with Erik off thread
who's been helping with code and test design.
Some of the main changes worthy of highlighting :
* Separate out the tests for JFR and Logger events
* Rename some events
* Normalize the data view for X509ValidationEvent (old event name :
CertChainEvent)
* Introduce CertificateSerialNumber type to make use of the @Relational
JFR annotation.
* Keep calls for JFR event operations local to call site to optimize jvm
compilation
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v6/webrev/
This code is dependent on the JDK-8203629 enhancement being integrated.
Regards,
Sean.
On 10/07/18 13:50, Seán Coffey wrote:
Erik,
After some trial edits, I'm not so sure if moving the event & logger
commit code into the class where it's used works too well after all.
In the code you suggested, there's an assumption that calls such as
EventHelper.certificateChain(..) are low cost. While that might be the
case here, it's something we can't always assume. It's called once for
the JFR operation and once for the Logger operation. That pattern
multiplies itself further in other Events. i.e. set up and assign the
variables for a JFR event without knowing whether they'll be needed
again for the Logger call. We could work around it by setting up some
local variables and re-using them in the Logger code but then, we're
back to where we were. The current EventHelper design eliminates this
problem and also helps to abstract the recording/logging functionality
away from the functionality of the class itself.
It also becomes a problem where we record events in multiple different
classes (e.g. SecurityPropertyEvent). While we could leave the code in
EventHelper for this case, we then have a mixed design pattern.
Are you ok with leaving as is for now? A future wish might be one
where JFR would handle Logger via its own framework/API in a future
JDK release.
I've removed the variable names using underscore. Also optimized some
variable assignments in X509Impl.commitEvent(..)
http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/
regards,
Sean.
On 09/07/2018 18:01, Seán Coffey wrote:
Erik,
Thanks for reviewing. Comments inline..
On 09/07/18 17:21, Erik Gahlin wrote:
Thanks Sean.
Some feedback on the code in the security libraries.
- We should use camel case naming convention for variables (not
underscore).
Sure. I see two offending variable names which I'll fix up.
- Looking at sun/security/ssl/Finished.java,
I wonder if it wouldn't be less code and more easy to read, if we
would commit the event in a local private function and then use the
EventHelper class for common functionality.
I'm fine with your suggested approach. I figured that tucking the
recording/logging logic away in a helper class also had benefits but
I'll re-factor to your suggested style unless I hear objections.
regards,
Sean.
