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.

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

Add getDefaultValue helper function #15030

Merged
merged 1 commit into from
May 18, 2022

Conversation

ehrenjulzert
Copy link

Signed-off-by: Ehren Julien-Neitzert ehren.julien-neitzert@ibm.com

@ehrenjulzert
Copy link
Author

@hangshao0
@a7ehuo does this look good to you? Do you want getDefaultValue added to cnathelp.cpp as well?

@hangshao0 hangshao0 added project:valhalla Used to track Project Valhalla related work comp:vm comp:jit labels May 9, 2022
runtime/vm/ValueTypeHelpers.cpp Outdated Show resolved Hide resolved
runtime/vm/ValueTypeHelpers.cpp Outdated Show resolved Hide resolved
runtime/vm/ValueTypeHelpers.cpp Outdated Show resolved Hide resolved
@a7ehuo
Copy link
Contributor

a7ehuo commented May 9, 2022

@ehrenjulzert Thank you for making this change!

@hzongaro and I have a question: Will clz->flattenedClassCache->defaultValue ever change due to GC?

@a7ehuo
Copy link
Contributor

a7ehuo commented May 9, 2022

Do you want getDefaultValue added to cnathelp.cpp as well?

I'm not 100% sure but I don't think it's required for the JIT

@hangshao0
Copy link
Contributor

Will clz->flattenedClassCache->defaultValue ever change due to GC?

It is possible that GC could change it. But @tajila or @dmitripivkine can confirm (or correct if I am wrong).

runtime/oti/j9nonbuilder.h Outdated Show resolved Hide resolved
runtime/vm/ValueTypeHelpers.cpp Show resolved Hide resolved
@dmitripivkine
Copy link
Contributor

Will clz->flattenedClassCache->defaultValue ever change due to GC?

It is possible that GC could change it. But @tajila or @dmitripivkine can confirm (or correct if I am wrong).

clz->flattenedClassCache->defaultValue is a slot containing object pointer. It can be changed by GC if object has been moved. This slot is one of slots returned by GC_ValueTypesIterator

@a7ehuo
Copy link
Contributor

a7ehuo commented May 10, 2022

clz->flattenedClassCache->defaultValue is a slot containing object pointer. It can be changed by GC if object has been moved.

If clz->flattenedClassCache->defaultValue can be changed, my understanding is that the JIT can't directly do a constant address load from clz->flattenedClassCache->defaultValue like below during the compile time (clz->flattenedClassCache->defaultValue = 0xf0043bf8). Maybe we need some kind of runtime helper or runtime indirection? Any thoughts @hzongaro @tajila?

n36n      treetop                             [0x7fe5ccb6d2c0] bci=[-1,24,26] rc=0 vc=0 vn=- li=- udi=- nc=1
n35n        aconst 0xf0043bf8                 [0x7fe5ccb6d270] bci=[-1,24,26] rc=2 vc=0 vn=- li=- udi=- nc=0

@tajila
Copy link
Contributor

tajila commented May 10, 2022

Maybe we need some kind of runtime helper or runtime indirection? Any thoughts @hzongaro @tajila?

Yes a runtime helper will work. We could also return the address, and you can handle the indirection in JIT code

@a7ehuo
Copy link
Contributor

a7ehuo commented May 10, 2022

you can handle the indirection in JIT code

@hzongaro @tajila Is there an existing example on how JIT handles such kind of indirection during the runtime in similar situation? I could experiment the indirection in the JIT code

@hzongaro
Copy link
Member

hzongaro commented May 11, 2022

you can handle the indirection in JIT code

Is there an existing example on how JIT handles such kind of indirection during the runtime in similar situation?

I haven't needed to do something like this before, but if you look at some of the methods in J9SymbolReferenceTable.cpp, like J9::SymbolReferenceTable::findOrCreateClassRomPtrSymbolRef, they create special shadow symbols that are used to access addresses of things.

So maybe if we had the helper return the address of the flattened class cache instead - let's say 0xC1A5CAC4E. The JIT could have a J9::SymbolReferenceTable::findOrCreateDefaultValuePtrSymbolRef method that it uses to create a shadow symbol for an address field, and it would set that shadow symbol's offset to be the offset of the defaultValue field inside of J9FlattenedClassCache (which happens to be zero today). Then the IL would look like:

n99n   aloadi <default-value>
n98n      aconst 0xC1A5CAC4E

@a7ehuo
Copy link
Contributor

a7ehuo commented May 11, 2022

So maybe if we had the helper return the address of the flattened class cache instead

Thanks for the example @hzongaro!

@hangshao0 @ehrenjulzert Since defaultValue changes due to GC, JIT should do a run time load off the flattened class cache. Could the API getDefaultValue be changed into getFlattenedClassCache which returns J9Class.flattenedClassCache instead assuming J9Class.flattenedClassCache will not change?

@hangshao0
Copy link
Contributor

assuming J9Class.flattenedClassCache will not change?

Correct, flattenedClassCache will not change.

@tajila
Copy link
Contributor

tajila commented May 11, 2022

assuming J9Class.flattenedClassCache will not change?

Correct, flattenedClassCache will not change.

The flattenedClassCache will not change, but I'd like to avoid increasing the cost of a potential refactor. What is the drawback of doing:

j9object_t*
getDefaultValueSlotAdddress(J9Class* claaz);

If the FCC is returned, you'd still have to do a deference

@hzongaro
Copy link
Member

What is the drawback of doing:

j9object_t*
getDefaultValueSlotAdddress(J9Class* claaz);

There is none really. Just looking at how the JIT gets various fields from J9Class, it looks like the JIT always takes the address of the J9Class and dereferences it using a symbol that has the offset of field of interest. I was imagining that if we might ever want to have the JIT get other things from the flattened class cache, it would be simplest if we got its address directly and did something similar for those other fields.

However, if you think it's unlikely that the JIT would ever need anything else that could conceivably be put into the flattened class cache, then returning the address of the default value slot would be fine.

@tajila
Copy link
Contributor

tajila commented May 12, 2022

However, if you think it's unlikely that the JIT would ever need anything else that could conceivably be put into the flattened class cache, then returning the address of the default value slot would be fine.

Im mostly worried about the cost of refactoring things in the FCC if we pass it out directly. It has already changed from its original format. So far the impact is contained to the VM. If we make it accessible outside the VM im worried that refactoring may involve more complexities.

Most of the other things in the FCC are already accessible via helpers (eg. getFlattenableFieldType, arrayElementSize, etc.)

@ehrenjulzert
Copy link
Author

I've updated the function signature to j9object_t* getDefaultValueSlotAdddress(J9Class* claaz);, although I'm still checking if the class is initialized and if defaultValue is null. Are these checks necessary anymore?

Copy link
Contributor

@hangshao0 hangshao0 left a comment

Choose a reason for hiding this comment

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

LGTM

@hangshao0 hangshao0 requested review from tajila and hzongaro May 17, 2022 15:03
runtime/oti/j9nonbuilder.h Outdated Show resolved Hide resolved
Signed-off-by: Ehren Julien-Neitzert <ehren.julien-neitzert@ibm.com>
@tajila
Copy link
Contributor

tajila commented May 17, 2022

Jenkins test sanity win jdk8

@tajila
Copy link
Contributor

tajila commented May 17, 2022

Jenkins test sanity,extended zlinuxval jdknext

@tajila
Copy link
Contributor

tajila commented May 17, 2022

Jenkins test sanity plinuxvalst jdknext

@tajila tajila merged commit eb81bd7 into eclipse-openj9:master May 18, 2022
@a7ehuo
Copy link
Contributor

a7ehuo commented May 26, 2022

@ehrenjulzert @hangshao0

I just realized getDefaultValueSlotAddress should not be guarded with J9VM_OPT_VALHALLA_VALUE_TYPES. The API needs to be available in non VT build because the JIT code doesn't have any build flag to exclude VT related code. I'm running into build failures in non-VT build. Sorry for catching it late.

Other helper calls used by JIT such as isClassRefQtype or isFlattenableFieldFlattened are also not guarded by J9VM_OPT_VALHALLA_VALUE_TYPES. Maybe something like below if it has be to be guarded?

j9object_t*
getDefaultValueSlotAddress(J9Class* clazz)
{
#if defined(J9VM_OPT_VALHALLA_VALUE_TYPES)
	Assert_VM_true(J9_IS_J9CLASS_VALUETYPE(clazz));
	Assert_VM_true(J9ClassInitSucceeded == clazz->initializeStatus); /* make sure class has been initialized (otherwise defaultValue won't exist) */
	j9object_t* result = &clazz->flattenedClassCache->defaultValue;
	Assert_VM_notNull(*result);
	return result;
#else 
        return NULL;
#endif /* #if defined(J9VM_OPT_VALHALLA_VALUE_TYPES) */
}

@tajila
Copy link
Contributor

tajila commented May 26, 2022

@ehrenjulzert can you make that change?

@ehrenjulzert
Copy link
Author

Sure, I can change that

@ehrenjulzert
Copy link
Author

New PR: #15132

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:jit comp:vm project:valhalla Used to track Project Valhalla related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants