Please review JDK-8035312

Attila Szegedi attila.szegedi at oracle.com
Wed Nov 12 11:00:35 UTC 2014


My remarks:

- NativeArray instances no longer need PushLinkLogic, PopLinkLogic, and ConcatLinkLogic objects. These objects are no longer instance-specific (their state is only in LinkLogic.switchpoints, but none of the ArrayLinkLogic subclasses override getOrCreateModificationSwitchpoint, so it's always null. You could just have NativeArray.getLinkLogic return static instances of these three that'd eliminate three reference fields from every array instance (similar to how CharCodeLinkingLogic only has one instance). 
- Related: LinkLogic switch points are no longer used, the logic for handling them should be removed (or moved into a patch until needed again).
- LengthNotWritableFilter(ArrayData, TreeMap) construtor shouldn't be copying the map; its invokers should ensure that data structures are copied when needed.
- LengthNotWritableFilter.sliceMap(from, to) is not needed, it should be replaced with simply new TreeMap(extraElements.subMap(from, to)).
- LengthNotWritableFilter.indexIterator() could just use "final List<Long> keys = computeIteratorKeys();" as the first line. The construction guarantees that the elements will be stricty monotonously ordered even after adding in the extraElements keySet. If needed, an "assert isStrictlyMonotonous(keys)" could be added, writing a simple linear function "boolean isStrictlyMonotonous(List<Long>)".

On Nov 12, 2014, at 11:41 AM, Hannes Wallnoefer <hannes.wallnoefer at oracle.com> wrote:

> A few minor remarks:
> 
> - Shouldn't ArrayData.increaseLength and .decreaseLength be protected instead of public?
> - Javadoc of ArrayData.length is missing # for link: {@link #setLength} (although it's a private field anyway...)
> - Is the (index >= len) check needed in NativeArray.sort? Does not ArrayData.indexIterator take care of this?
> - Some of the new tests use a mix of spaces/tabs for indentation.
> 
> Otherwise looks good.
> 
> Hannes
> 
> Am 2014-11-11 um 17:37 schrieb Marcus Lagergren:
>> Please review
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8035312 <https://bugs.openjdk.java.net/browse/JDK-8035312>
>> 
>> There were several corner cases related to length in general and setting length in arrays to not writable in particular.
>> None of the existing run times pass all the tests, so this was a very hard area to get right (added 6 new unit tests)
>> 
>> I’ve also gotten rid of the special casey length not writable SwitchPoint in NativeArray - now that I have a filter for LengtNotWritableArray that can’t be cast to a ContinuousArrayData in the fast paths, this handles itself anyway.
>> 
>> webrev at: http://cr.openjdk.java.net/~lagergren/8035312/
>> 
>> /M
>> 
> 



More information about the nashorn-dev mailing list