Avoid hidden exceptions from Lookup.unreflectSpecial(…) in default method invocations [DATACMNS-1074]

[spring-projects-issues]

Christoph Dreis opened DATACMNS-1074 and commented

Hey,

Preface

with DATACMNS-867 and DATACMNS-944 not only the naming schemes changed, but also Optional was introduced as return type in CrudRepository.findOne or respectively CrudRepository.findById. While I certainly like the new naming scheme, which I could easily replace with a regex, the change to Optional changes semantics quite a bit and creates a lot of boilerplate code in my opinion as one has to call the debated Optional.get() everytime when not used in a stream - which I think is still the more common case. That effectively replaces a NullPointerException with a NoSuchElementException if not handled otherwise.

Actual problem

That being said, I do understand the wish to return Optional to enhance the Stream support. So I don't mind and see it as a migration effort to change to the new methods. To give me some time for that I introduced a default method in my projects BaseRepository similar to the following.

interface CityRepository extends CrudRepository<City, Long> {

	default City findOne(Long id) {
		return findById(id).orElse(null);
	}

}

As you see I'm simply dispatching to the new methods and call Optional.orElse(null) to get the previous behaviour. This helps a lot with migrating step by step in my big project. So far so good.

The problem with that though is, that for default methods MethodHandles$Lookup.unreflectSpecial() is called in DefaultMethodInvokingMethodInterceptor.invoke(MethodInvocation) which produces lots and lots of "silent" exceptions. In a 10 minute profiling I have 1.2 million(!) exceptions as shown in the attached screenshot (both come from the same stack).

!exceptions-spring-data-default.jpg|thumbnail!

This adds quite a bit of pressure on the JVM and costs performance.

I wonder if we could solve/enhance that somehow to ease the migration process for people and don't punish them with a performance degradation?

Cheers,
Christoph

---

Affects: 1.13.1 (Ingalls SR1), 1.12.8 (Hopper SR8), 2.0 M3 (Kay)

Attachments:

• exceptions-spring-data-default.jpg (170.63 kB)

Referenced from: pull request #221

Backported to: 1.13.4 (Ingalls SR4), 1.12.11 (Hopper SR11)

[spring-projects-issues]

Oliver Drotbohm commented

Can we clarify, what the change in Optional has to do with the invocation of default methods?

Generally speaking, a migration of existing projects to Spring Data 2.0 is not something we recommend as you will need to refactor a lot of code. That said, calling get() on Optional is quite a mistake, as you'd rather call map(…) or ifPresent(…) on it. I know this means touching client code, but it's a major version bump which is our only chance to introduce such changes. Lifting a codebase to JDK 8 is going to cause effort but we have start at some point.

That said, let Mark investigate the exceptions and see what we can do. I wonder what might have changed so that the exceptions are produced now and haven't been on Ingalls. We need to fix that either way

[spring-projects-issues]

Christoph Dreis commented

Hey,

thanks for the quick response.

Without the introduction of Optional I wouldn't need to introduce this default method and simply replace all findOne occurences with findById without a semantic change. I wanted to explain the reasoning behind my ticket - without huge success apparently ;)

Your absolutely right about get() being wrong. Hence my comment about being it debated. map(…) or ifPresent(…) or even orElseThrow are certainly more useful in most cases, but again creates quite a lot of boilerplate in my opinion for non stream usages. I do understand the reasoning though - as I said.

I want to clarify that we're talking about silent exceptions that are never surfacing to the user. So I don't know if there are really that new or were simply never noticed.

[spring-projects-issues]

Mark Paluch commented

The JDK uses internally exceptions as control flow to resolve member names during MethodHandle lookup.

That behavior exists since Java 8 and is not related to our changes. Our DefaultMethodInvokingMethodInterceptor performs a MethodHandle lookup (construct method handle from a reflection Method) each time a default method is invoked.

It would make sense to cache the resulting MethodHandle

[spring-projects-issues]

Christoph Dreis commented

+1 for the caching idea. Should reduce the reflection and the exception overhead better