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

Deprecate ShrinkWrapping - Part III #3291

Merged

Conversation

fjeremic
Copy link
Contributor

Remove some last bits and pieces of Shrink Wrapping code in the stack
walker.

Issue: eclipse/omr#2107

Signed-off-by: Filip Jeremic fjeremic@ca.ibm.com

@fjeremic
Copy link
Contributor Author

Changes are rather simple but wouldn't hurt a review from either @gacholio or @keithc-ca given changes in their area of expertise. FYI this removes the last bit of code related to shrink wrapping in OpenJ9 (AFAIK).

@gacholio
Copy link
Contributor

Looks reasonable to me - I don't think you should edit the DDR artifacts. @keithc-ca ?

@fjeremic
Copy link
Contributor Author

Looks reasonable to me - I don't think you should edit the DDR artifacts. @keithc-ca ?

Yeah I was wondering about that. Is there a reason for those files to exist in OpenJ9? There seems to be like 40 spec files each with over 10k lines of "stuff". That's more lines together than the entire JIT compiler for example. Takes up a few mbs of space too in the repo.

@keithc-ca
Copy link
Contributor

The changes to files in DDR_Artifacts have no effect on OpenJ9 builds, they're only relevant to the legacy tooling used in IBM builds and they're not needed (or wanted) there.

@keithc-ca
Copy link
Contributor

I don't know what 'shrink wrapping' means in this context, but if any JVM based on OpenJ9 exists that might generate a core file to be examined, the support for interpreting such core files should remain.

@fjeremic
Copy link
Contributor Author

fjeremic commented Oct 16, 2018

The changes to files in DDR_Artifacts have no effect on OpenJ9 builds, they're only relevant to the legacy tooling used in IBM builds and they're not needed (or wanted) there.

Ok, I'll revert the spec changes then. On another note if these files have no effect on OpenJ9 builds can we migrate them to the IBM internal repo that cares about them? Why pollute the open source repo if the files are meaningless? Should we open up an issue for this? If you can specify which files can be migrated I will gladly do the work.

Edit:

It also makes filtering through the noise harder when searching for something in the repo as for pretty much any C++ entity there will be a search result in each and every one of these spec files.

@fjeremic fjeremic force-pushed the remove-shrink-wrapping-part-iii branch from b853987 to 7d4469a Compare October 16, 2018 14:03
@fjeremic
Copy link
Contributor Author

Jenkins test sanity all JDK8

* the save offset is located from bits 18-32
* so first mask it off to get the bit vector
* corresponding to the saved GPRS
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please fix the indentation of this comment so is consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 19913fe

@fjeremic fjeremic force-pushed the remove-shrink-wrapping-part-iii branch from 7d4469a to 19913fe Compare October 16, 2018 19:50
Remove some last bits and pieces of Shrink Wrapping code in the stack
walker.

Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
@0xdaryl
Copy link
Contributor

0xdaryl commented Oct 16, 2018

Jenkins test sanity all JDK11

Launching again because the record of whether the previous PR build was successful was lost on the last push, and I did not see the results.

@0xdaryl 0xdaryl merged commit 48a60b6 into eclipse-openj9:master Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants