Skip to content

Conversation

@yrodiere
Copy link
Member

@yrodiere yrodiere commented Jan 6, 2025

https://hibernate.atlassian.net/browse/HHH-18976

Two things in here:

  1. Sets up forbiddenapis to forbid usage of Array.newInstance (and util methods known to use it) by default. This is mostly as a heads-up to not use it "just because", since the check can be bypassed if necessary by annotating the method with @AllowReflection.
  2. Reworks a few implementations to avoid usage of Array.newInstance. E.g.:
    • use array.clone() where relevant (i.e. when we want the same array type with the same length).
    • don't use Array.newInstance just to retrieve the Class of an array type -- since we ultimately just look use that Class to look up a descriptor in a registry.
    • use new Object[...] instead where possible (e.g. when the array ends up being used internally only).

This should get rid of most problematic uses of reflection when compiling to native binaries (see Jira issue).
There are still a few uses of Array.newInstance but as far as I can see they are completely justified and cannot easily be worked around: array mapping, JSON/XML serialization, instantiation of query result types, ...

…lans

It's more consistent, and happens to get rid of ArrayMutabilityPlan,
which involved an unnecessary use of Array.newInstance.

I've also seen claims that clone() performs better than
Array.newInstance() due to not having to zero-out the allocated memory,
but I doubt that's relevant here.
No functional impact, it's just less redundant.
… to new Object[]

The dynamic instantiations were originally introduced to fix
the following issues:

* HHH-16466 (initial support for array parameters in multi-key loads) --
  tested in MultiLoadTest
* HHH-16750 -- tested in CompositeIdWithOneFieldAndElementCollectionBatchingTest
* HHH-17201 -- tested in MultiIdEntityLoadTests

The corresponding tests still pass after removing these dynamic array
instantiations.
@yrodiere
Copy link
Member Author

yrodiere commented Jan 6, 2025

Hey @sebersole @beikov, this might be of interest to you. I'm still waiting for the full CI run -- I personally only ran tests in a few relevant test packages. But assuming tests pass, I think merging this would make HHH-16809 / #6815 mostly irrelevant... A JavaType#createArray method could still be useful in the future, but more for performance purposes (e.g. use primitive arrays) that would require more extensive changes.

EDIT: FWIW: HHH-18976...HHH-16809. It's unfinished, I didn't implement newArray (renamed from createArray) everywhere, but you can at least see where it would get used.

@yrodiere
Copy link
Member Author

yrodiere commented Jan 6, 2025

So I thought this, given the implementation of H2Dialect, meant arrays were in use for H2 as well:

protected MultiIdEntityLoader<Object> buildMultiIdLoader() {

But apparently they are not, so the successful tests I had locally were meaningless and I need to test on postgres specifically.

Back to the drawing board...

EDIT:

@Override
public boolean useArrayForMultiValuedParameters() {
// Performance is worse than the in-predicate version
return false;
}
-_-

@yrodiere
Copy link
Member Author

yrodiere commented Jan 8, 2025

Closing, will submit another PR with a more conservative solution.

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.

1 participant