-
Notifications
You must be signed in to change notification settings - Fork 0
Preview mode changes all-in-one. #1
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
base: jdk_8352750_baseline
Are you sure you want to change the base?
Conversation
Reviewed-by: thartmann, mhaessig, dfenacci
…offset 8 to null pointer Reviewed-by: mdoerr, lucy
Reviewed-by: ihse, lucy
Reviewed-by: sangheki, kbarrett, tschatzl
Reviewed-by: tschatzl
Reviewed-by: kbarrett, ayang
…hat no proxy is selected Reviewed-by: lancea, iris, joehw
Reviewed-by: kvn, syan, dholmes
…le race Reviewed-by: dfuchs, vyazici
Reviewed-by: sviswanathan, sparasa, epeter
Reviewed-by: lmesnik, dholmes, sspitsyn
…nts::clearall_in_class_at_safepoint Reviewed-by: coleenp, dholmes, sspitsyn
Reviewed-by: amenkov, dholmes, sspitsyn
Reviewed-by: hchao, mullan, shade
…pected output Reviewed-by: shade, iklam
Reviewed-by: ccheung, kvn, dholmes
Reviewed-by: aph, tschatzl
…at usage Reviewed-by: stefank, dholmes
… format string Reviewed-by: kvn, mhaessig, yzheng
…n AquaL&F Reviewed-by: dnguyen, abhiscxk
Reviewed-by: ccheung
…ches Reviewed-by: kvn, thartmann
Reviewed-by: coleenp, sspitsyn
Reviewed-by: kvn, dfenacci, mdoerr
…e macro elimination Reviewed-by: kvn, dlunden
Reviewed-by: mhaessig, thartmann
Reviewed-by: kvn, shade
Reviewed-by: tschatzl, kbarrett
Reviewed-by: liach, rgiulietti
Co-authored-by: Alan Bateman <[email protected]> Reviewed-by: alanb, jkern
…ption Reviewed-by: phubner
…ng convention Co-authored-by: Tobias Hartmann <[email protected]> Reviewed-by: thartmann, coleenp
Reviewed-by: dsimms
…ring.java fails to boot with genzgc Reviewed-by: coleenp
Reviewed-by: liach
Reviewed-by: coleenp
…9370 Reviewed-by: liach, mcimadamore
…r insn" Reviewed-by: thartmann, chagedorn
…zed' Co-authored-by: Chen Liang <[email protected]> Reviewed-by: fparain
…ectHeaders and preview mode Reviewed-by: chagedorn, matsaave
…rashes with SIGSEGV Reviewed-by: lfoltan
…dingTest.java crashes with segmentation fault Reviewed-by: coleenp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round of comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore this, it's just a fix I have in my local Valhalla repo for using git workspaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to get the preview classes into the jimage file by moving the compiled classes into the "META-INF/preview" module subdirectory. It seems to work fine (and has been for months).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added for testing ImageReader properly.
return SymbolTable::new_symbol(name, pointer_delta_as_int(start, base), pointer_delta_as_int(end, base)); | ||
} | ||
|
||
// -------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added these "micro functions" to just encapsulate everywhere that JImage_file and JImage_mode were used. There's now more complexity than just whether JImage_file is null or not.
Some of these could be considered too small to carry their own weight, but I think it's nice not to see these fields accessed anywhere outside of this code section where they could be used in an invalid way.
} | ||
FreeHeap(valueclasses_dir); | ||
} | ||
// // For each <module>-valueclasses.jar in <JAVA_HOME>/lib/valueclasses/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We MUST NOT submit this as is because it would break the module patching.
We need to figure out what to do here to make a clean transition in this code and the C++ and makefiles.
* @param nameDecoder decoder for module names. | ||
* @return the list of module references for the source package subdirectory. | ||
*/ | ||
public static List<ModuleReference> read( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useful to have this here since it touches the flags (and I wanted to keep those private to avoid confusion between this and the ImageLocation flags). This is called by ImageReader when it "completes" package directories and when it processes preview mode packages.
Looking at it, I think it should be re-worked to not actually return the ref instances since the callers care only about the names.
@@ -0,0 +1,77 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Figure out what to do about ImageReaderFactory.java.
A rename of ImageReaderFactory.java, not sure why it's not showing up as a such here however. I need to make sure that the old class is gone.
@@ -1,5 +1,5 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Figure out what's going on here.
This doesn't handle preview mode yet. I've some code somewhere that should enable it, but I'll have to find it. I think the changes in here are really what's in a mainline PR for exploded image, but I've lost track a bit.
size_t index; | ||
char name_buffer[IMAGE_MAX_PATH]; | ||
char* path; | ||
{ // Write the buffer with room to prepend the preview mode infix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using an anonymous scope just to avoid index from needing to be a live variable anywhere else.
index += name_len; | ||
name_buffer[index++] = '\0'; | ||
// Path begins at the leading '/' (not the start of the buffer). | ||
path = &name_buffer[preview_infix_len]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit janky to have my string start some way into the buffer, but it avoid copying the path, which is typically longer than the module name by some margin.
…ion errors Reviewed-by: coleenp
Reviewed-by: fparain
fe0bd75
to
ee3dbcf
Compare
Reviewed-by: coleenp
…halla Reviewed-by: liach, rriggs
* Fix test for new ModuleReference behaviour. * Make preview mode system property dependent in classloader. * Update benchmark for preview mode and new ImageReader methods. * Adjusting tests and fixing loose ends from merge. * Serialization fix * squash everything latest
58736f7
to
3212420
Compare
THIS IS NOT FOR CODE REVIEW - DO NOT REVIEW THIS
Rollup of all current preview mode work for Valhalla.
Because the work I've put into JDK mainline (as far back as July) is not yet integrated into Valhalla, this work cannot be considered for submission yet. However, I'm going to put it here for specific people to look at.
Because this is based off a baseline of the changes in mainline (manually patched in), it MUST be view in relation to openjdk#1596 to get the preview mode specific diffs.
jdk_8352750_baseline...david-beaumont:openjdk_valhalla:jdk_8352750_preview_mode