[NETBEANS-4123] Initial implementation of handling large strings

[SirIntellegence]

in Java 9+ where the String class uses a byte[] instead of a char[]

Not a final implementation at the moment, but should be enough for now. I didn't see any tests related to this class, so I didn't write any. I have tested it and it works properly. May need more testing.

Handling UTF16 byte[] strings would require more effort since it becomes a nightmare.

[SirIntellegence]

This is intended to (mostly) fix #4123
Mostly because I am not sure how to handle the UTF16 stuff for the reasons mentioned in the source file.

[matthiasblaesing]

Please add the necessary imports - the proposed changes also need corresponding import adjustments. Thank you.

[SirIntellegence]

Ya. I am aware. I am still working in it. I did find out how to grab the hi byte field, so I added logic in for that. I will get to that tomorrow.

[matthiasblaesing]

I think this should be cacheable with a WeakHashMap using the Virtualmachine as Key. This might be slow if the target VM is not local. I experimented a bit with this code and on JDK 17 initially possibleClasses was empty for me. Not sure how the VM handled this interernally, but I could make it work using this code:

        if (possibleClasses.isEmpty()) {
            ClassType ct = (ClassType) virtualMachine.classesByName("java.lang.Class").iterator().next();
            Method m = ct.concreteMethodByName("forName", "(Ljava/lang/String;)Ljava/lang/Class;");
            StringReference referenceString = virtualMachine.mirrorOf("java.lang.StringUTF16");
            try {
                ThreadReference threadReference = virtualMachine.allThreads().get(0);
                ct.invokeMethod(threadReference, m, Collections.singletonList(referenceString), 0);
                possibleClasses = virtualMachine.classesByName("java.lang.StringUTF16");
            } catch (InvalidTypeException | ClassNotLoadedException | IncompatibleThreadStateException | InvocationException ex) {
                Exceptions.printStackTrace(ex);
            }
        }

[SirIntellegence]

Aye. Will get to that. Do we need to handle multiple threads or are we good without using concurrent structures here?

[matthiasblaesing]

In my test I had to make isSmallEndian synchronized - else I got into a IncompatibleThreadStateException. Not really sure whether this hammer just fixed the problem accidentally.

[SirIntellegence]

Ok. Should I slap a synchronized onto it, not worry about it, or find another solution?

[matthiasblaesing]

Works for me and looks sane. The caching you introduced works (at least in my trivial testcase).

One final idea would be to rework the synchronization. The maps are protected by individual sychronized() {} blocks. The pattern could be applied here two, which would also not leak the sychronization to the outside.

My suggestion is to move the isLittleEndianCache.clear(); into a separate block synchronized (isLittleEndianCache) {isLittleEndianCache.clear();}, drop the synchronized from the signature of isLittleEndian and instead wrap the whole method contents into a synchronized (isLittleEndianCache) {....} block.

[matthiasblaesing]

Final cleanup (see inline comments). Please also squash the commits into a single one.

[SirIntellegence]

While applying those changes, noticed that getStringWithLengthControl is declared to throw some exceptions. Should isLittleEndian throw the internal exceptions as well instead of catching them?

[matthiasblaesing]

While applying those changes, noticed that getStringWithLengthControl is declared to throw some exceptions. Should isLittleEndian throw the internal exceptions as well instead of catching them?

I have mixed feelings here. The use sites don't give information about what went wrong, which would make it hard to debug. What would you think about this idea: let isLittleEndian throw all its exceptions, but add a multi catch for these before the finally-clause before 367, log errors there and rethrow. I would then not use Exceptions#printStackTrace, but a java.util.Logger on level INFO. printStackTrace creates useless message window.

[SirIntellegence]

Pushing the exception lift change so it can be reviewed before squishing everything

[matthiasblaesing]

To me this looks sane, thank you. It is an impressive list of Exceptions and we might want to refactor these calls later, but I think as a fix for the issue at hand this should be merged.

I requested a first CI run and the system is happy. So, I suggest that you squash the commit as you see fit. I would suggest to squash into a single commit as all commit cover the same area and there are not really smaller building blocks. If then GH actions is happy, I'll merge.

[SirIntellegence]

And squashed

[matthiasblaesing]

Thank you. As the unittests finally run cleanly through, lets get this in.