Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ValidationException performance cost #318

Closed
rpolyano opened this issue Jul 29, 2019 · 4 comments
Closed

ValidationException performance cost #318

rpolyano opened this issue Jul 29, 2019 · 4 comments

Comments

@rpolyano
Copy link

rpolyano commented Jul 29, 2019 •
edited
Loading

Hello,

I'm using this library to validate elements against a large schema (https://www.hl7.org/fhir/downloads.html - https://www.hl7.org/fhir/fhir.schema.json.zip). We noticed a significant (almost 10x!) performance cost when using the validator in our program - mostly because our program has a large-ish stack (or maybe there is some other aspect, other than size of our stack that is causing problems, like asynchronicity) when it does validation, so java.lang.Throwable#fillInStackTrace takes a "long time" to run. Forking the code, and adding

@Override
public synchronized Throwable fillInStackTrace() {
  return this;
}

to ValidationException improves performance by almost 10x in our specific application.

Obviously this is not likely to be a good permanent solution (there are probably scenarios where the stack is useful) but being able to trigger this behaviour behind a flag or something would be ideal.

What do y'all think? Is this something we can optimize in the library, or maybe externally at the callsite?
erosb added a commit that referenced this issue Sep 12, 2019
@erosb
omitting stacktraces for internal exceptions
41dcbd3 
The purpose of this change is to avoid stacktrace generation for most `ValidationException` instances, since it can
come with significant performance improvements in some usecases, see #318 .

An `InternalValidationException` is introduced which is a subclass of `ValidationException` but it omits the stacktrace
generation by overriding `fillInStackTrace()` to be no-op. This is now thrown everywhere internally. At the end of the
validation process the `DefaultValidator` catches the possibly thrown `InternalValidationException` and copies it into a
`ValidationInstance` and throws it, so from the caller point of view an exception with a proper stacktrace is thrown. This
"public" `ValidationException` is necessary to avoid any potential BC breaks.

Tests updated. A good number of unittests and all of the integration tests verify that only root exceptions (and no causing
exceptions) have non-empty stacktraces. There is some code duplication between the unit and integ test codebase, this is
to be cleaned up a bit in the future (needs a separate testutil module which can be depended on by both `core` and
`tests/vanilla`).
@erosb
Copy link
Contributor

erosb commented Sep 12, 2019

Hello @rpolyano, this is a very cool idea. Implemented in 41dcbd3

The ValidationException thrown back to the library caller still contains a proper stacktrace, but for the internally caught exceptions it is suppressed in the way you suggested.

@rpolyano
Copy link
Author

Fantastic, thank you! Hopefully this improves performance for others too.

@erosb
Copy link
Contributor

erosb commented Sep 20, 2019

This is now fixed and released in version 1.12.0.
@grahamegrieve you may also be interested in this, it can speed up your build process.

@grahamegrieve
Copy link

thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@erosb @grahamegrieve @rpolyano