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

Prevent maxAlign virtual calls for polluted accesses #700

Closed
wants to merge 2 commits into from
+107 −46
Closed

Prevent maxAlign virtual calls for polluted accesses #700

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f276c4
Prevent maxAlign virtual calls for polluted accesses
rsmogura Aug 6, 2022
aadb898
Previous version created performance drop for `LoopOverNonConstantHea…
rsmogura Aug 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
View file
Original file line number Diff line number Diff line change
@@ -66,22 +66,23 @@
* {@link MappedMemorySegmentImpl}.
*/
public abstract non-sealed class AbstractMemorySegmentImpl implements MemorySegment, SegmentAllocator, Scoped, BiFunction<String, List<Number>, RuntimeException> {
private static final long MAX_ALIGN_1 = 1;
private static final long MAX_ALIGN_2 = 2;
private static final long MAX_ALIGN_4 = 4;
private static final long MAX_ALIGN_8 = 8;

private static final ScopedMemoryAccess SCOPED_MEMORY_ACCESS = ScopedMemoryAccess.getScopedMemoryAccess();

private final long maxAlignMask;

static final JavaNioAccess nioAccess = SharedSecrets.getJavaNioAccess();

final long length;
final boolean readOnly;
final MemorySession session;

@ForceInline
AbstractMemorySegmentImpl(long length, boolean readOnly, long maxAlignMask, MemorySession session) {
AbstractMemorySegmentImpl(long length, boolean readOnly, MemorySession session) {
this.length = length;
this.readOnly = readOnly;
this.maxAlignMask = maxAlignMask;
this.session = session;
}

@@ -322,8 +323,26 @@ public void checkValidState() {

// Helper methods

@ForceInline
Copy link
Collaborator

@mcimadamore mcimadamore Aug 8, 2022 •
edited by bridgekeeper bot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When adding maxAlign I have played with something similar as what you have done. My recollection is that doing this doesn't 100% solve the issue, as it's possible to have polluted profile for maxAlignMask (e.g. the JVM will bias the method implementation towards the 1-2 layouts that seem to be more common, and put everything else in an uncommon branch). Maybe what you see is that, since the virtual call is gone, there is still a net gain.

Does the problem only manifest with bulk copy? Or also with plain memory access? If the former, perhaps some other inlining issue with bulk copy (which is a big method) could be at play.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked it with vector operations only, there's check in ScopedMemoryAccess loadFromMemorySegment which checks max align in case of polluted access this will create polymorphic call.

  • However when I look right now I think there's second option. Instead of changing maxAlign, the check in loadFromMemorySegment, could be replaced by checking if segment instance is native or Bytes.?

Copy link
Collaborator

@mcimadamore mcimadamore Aug 8, 2022 •
edited by bridgekeeper bot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the affected code?

// @@@ Smarter alignment checking if accessing heap segment backing non-byte[] array
        if (msp.maxAlignMask() > 1) {
            throw new IllegalArgumentException();
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly!

Copy link
Collaborator

@mcimadamore mcimadamore Aug 8, 2022 •
edited by bridgekeeper bot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess which solution is taken depends on how deep of a problem this is. I suggest writing a benchmark using e.g. bulk copy between segments, and see if the same issue occurs. We do have tests for polluted memory segment access and we have not seen issues with maxAlignMask showing up there, so I'm curious as to why the vector use-site seems to be more problematic. I wonder if part of the issue is the lack of argument type profiling - e.g. profile info based on the type of arguments in a static call. I think Method/VarHandle have that enabled by default (for obvious reasons) and there are hacks in the JVM to allow that for Unsafe as well. But that support is not enabled for this particular vector call, and I wonder if that could lead to the problem you describe.

https://github.com/openjdk/jdk/blob/master/src/hotspot/share/oops/methodData.cpp#L1583

Copy link
Member

@PaulSandoz PaulSandoz Aug 15, 2022 •
edited by bridgekeeper bot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expanding the profiling to for method names starting with "loadFrom" and "storeInto" seems reasonable, given the most other access methods are also profiled (although not all the read-modify-write methods are).

I think we should separate out the comparison of method names thereby we don't inadvertently profile other methods just in case new methods get added. Plus we can add some comments to the class doc of Unsafe and ScopedMemoryAccess to indicate such methods are profiled. Ideally I would prefer an HotSpot-specific annotation indicated that all arguments of an annotated method should be profiled.

The profiling of all arguments is a blunter hammer that necessary in this case but so be it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding annotation for this could be useful for profiling other methods too. I'll try to find some time and prepare some draft.

One thing I see that adding such annotation could consume one bit here (still there will be 5 spare):
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/oops/method.hpp#L98

Copy link
Member

@PaulSandoz PaulSandoz Aug 16, 2022 •
edited by bridgekeeper bot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I think it may be better to propose the more focused fix to JDK 20, and then use that PR as the opportunity to ask the question to the HotSpot devs whether they think an annotation would be useful before diving in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you are suggesting to create PR with annotations and ask on hotspot-dev what they think about it?

Copy link
Member

@PaulSandoz PaulSandoz Aug 17, 2022 •
edited by bridgekeeper bot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, i am suggesting to create a PR with changes to https://github.com/openjdk/jdk/blob/master/src/hotspot/share/oops/methodData.cpp#L1583 as you proposed. Then before you dive into any annotation driven approach ask on the hotspot email list, or ask in that PR.

public final long maxAlignMask() {
return maxAlignMask;
if (this instanceof NativeMemorySegmentImpl) {
return 0;
} else if (this instanceof HeapMemorySegmentImpl.OfByte) {
return MAX_ALIGN_1;
} else if (this instanceof HeapMemorySegmentImpl.OfChar) {
return MAX_ALIGN_2;
} else if (this instanceof HeapMemorySegmentImpl.OfShort) {
return MAX_ALIGN_2;
} else if (this instanceof HeapMemorySegmentImpl.OfInt) {
return MAX_ALIGN_4;
} else if (this instanceof HeapMemorySegmentImpl.OfLong) {
return MAX_ALIGN_8;
} else if (this instanceof HeapMemorySegmentImpl.OfFloat) {
return MAX_ALIGN_4;
} else if (this instanceof HeapMemorySegmentImpl.OfDouble) {
return MAX_ALIGN_8;
}
throw new IllegalStateException("Unknown memory segment");
}

@ForceInline
23 changes: 9 additions & 14 deletions src/java.base/share/classes/jdk/internal/foreign/HeapMemorySegmentImpl.java
View file
Original file line number Diff line number Diff line change
@@ -53,11 +53,6 @@ public abstract class HeapMemorySegmentImpl extends AbstractMemorySegmentImpl {
private static final Unsafe UNSAFE = Unsafe.getUnsafe();
private static final int BYTE_ARR_BASE = UNSAFE.arrayBaseOffset(byte[].class);

private static final long MAX_ALIGN_1 = 1;
private static final long MAX_ALIGN_2 = 2;
private static final long MAX_ALIGN_4 = 4;
private static final long MAX_ALIGN_8 = 8;

final long offset;
final Object base;

@@ -67,8 +62,8 @@ public Optional<Object> array() {
}

@ForceInline
HeapMemorySegmentImpl(long offset, Object base, long length, boolean readOnly, long maxAlignMask) {
super(length, readOnly, maxAlignMask, MemorySession.global());
HeapMemorySegmentImpl(long offset, Object base, long length, boolean readOnly) {
super(length, readOnly, MemorySession.global());
this.offset = offset;
this.base = base;
}
@@ -95,7 +90,7 @@ ByteBuffer makeByteBuffer() {
public static class OfByte extends HeapMemorySegmentImpl {

OfByte(long offset, Object base, long length, boolean readOnly) {
super(offset, base, length, readOnly, MAX_ALIGN_1);
super(offset, base, length, readOnly);
}

@Override
@@ -123,7 +118,7 @@ public long address() {
public static class OfChar extends HeapMemorySegmentImpl {

OfChar(long offset, Object base, long length, boolean readOnly) {
super(offset, base, length, readOnly, MAX_ALIGN_2);
super(offset, base, length, readOnly);
}

@Override
@@ -151,7 +146,7 @@ public long address() {
public static class OfShort extends HeapMemorySegmentImpl {

OfShort(long offset, Object base, long length, boolean readOnly) {
super(offset, base, length, readOnly, MAX_ALIGN_2);
super(offset, base, length, readOnly);
}

@Override
@@ -179,7 +174,7 @@ public long address() {
public static class OfInt extends HeapMemorySegmentImpl {

OfInt(long offset, Object base, long length, boolean readOnly) {
super(offset, base, length, readOnly, MAX_ALIGN_4);
super(offset, base, length, readOnly);
}

@Override
@@ -207,7 +202,7 @@ public long address() {
public static class OfLong extends HeapMemorySegmentImpl {

OfLong(long offset, Object base, long length, boolean readOnly) {
super(offset, base, length, readOnly, MAX_ALIGN_8);
super(offset, base, length, readOnly);
}

@Override
@@ -235,7 +230,7 @@ public long address() {
public static class OfFloat extends HeapMemorySegmentImpl {

OfFloat(long offset, Object base, long length, boolean readOnly) {
super(offset, base, length, readOnly, MAX_ALIGN_4);
super(offset, base, length, readOnly);
}

@Override
@@ -263,7 +258,7 @@ public long address() {
public static class OfDouble extends HeapMemorySegmentImpl {

OfDouble(long offset, Object base, long length, boolean readOnly) {
super(offset, base, length, readOnly, MAX_ALIGN_8);
super(offset, base, length, readOnly);
}

@Override
2 changes: 1 addition & 1 deletion src/java.base/share/classes/jdk/internal/foreign/NativeMemorySegmentImpl.java
View file
Original file line number Diff line number Diff line change
@@ -64,7 +64,7 @@ public Optional<Object> array() {

@ForceInline
NativeMemorySegmentImpl(long min, long length, boolean readOnly, MemorySession session) {
super(length, readOnly, 0, session);
super(length, readOnly, session);
this.min = min;
}

25 changes: 25 additions & 0 deletions test/micro/org/openjdk/bench/jdk/incubator/vector/MixedAccessBenchmarks.java
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,28 @@
/*
* Copyright (c) 2022, Rado Smogura
*
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the "Classpath" exception as provided
* by Oracle in the LICENSE file that accompanied this code.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package org.openjdk.bench.jdk.incubator.vector;

import jdk.incubator.vector.ByteVector;