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

Jump to bottom

Create more detailed OSR documentation #7200

Merged
merged 1 commit into from
Feb 1, 2024
Merged

Create more detailed OSR documentation #7200

merged 1 commit into from
Feb 1, 2024

Conversation

gita-omr
Copy link
Contributor

@gita-omr gita-omr commented Dec 11, 2023 •
edited
Loading

  • introduce new concepts
  • provide detailed desciption of OSR mechanism based on involuntary OSR
  • map new concepts into existing API

@gita-omr
Copy link
Contributor Author

Added commit message.

@gita-omr gita-omr force-pushed the osr_doc branch 2 times, most recently from 1e21a23 to aaad89f Compare December 11, 2023 19:16
@gita-omr
Copy link
Contributor Author

Removed empty lines a the EOF.

hzongaro
hzongaro reviewed Dec 19, 2023
View reviewed changes
Copy link
Contributor

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

Started reviewing, but had to put it aside. I thought I should pass along my comments thus far, so they don't get lost.

doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
@gita-omr
Copy link
Contributor Author

Thanks @hzongaro for the review. I will address it soon. @jdmpapin could you please review as well?

jdmpapin
jdmpapin reviewed Dec 22, 2023
View reviewed changes
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
@gita-omr
Copy link
Contributor Author

gita-omr commented Jan 2, 2024

Addressed all comments above.

@gita-omr
Copy link
Contributor Author

gita-omr commented Jan 2, 2024

I think I missed a bunch of comments. Looking at them now...

@gita-omr
Copy link
Contributor Author

gita-omr commented Jan 3, 2024

Addressed all the comments.

@gita-omr
Copy link
Contributor Author

gita-omr commented Jan 5, 2024

Fixed line ending.

jdmpapin
jdmpapin reviewed Jan 8, 2024
View reviewed changes
Copy link
Contributor

@jdmpapin jdmpapin left a comment

Choose a reason for hiding this comment

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

A few more minor copy edits

doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
@gita-omr
Copy link
Contributor Author

gita-omr commented Jan 9, 2024

Thanks a lot for the corrections. Addressed.

@jdmpapin
Copy link
Contributor

jdmpapin commented Jan 9, 2024

Thanks!

I think you might have missed my replies to earlier threads:

@gita-omr
Copy link
Contributor Author

gita-omr commented Jan 9, 2024

Addressed 3 comments above but #7200 (comment) still needs some discussion.

@gita-omr gita-omr force-pushed the osr_doc branch 2 times, most recently from 6fcd9e2 to c42e33b Compare January 11, 2024 22:59
@gita-omr
Copy link
Contributor Author

Addressed #7200 (comment)

@gita-omr
Copy link
Contributor Author

Pushed minor correction.

@gita-omr
Copy link
Contributor Author

Pushed more corrections.

@gita-omr
Copy link
Contributor Author

Pushed more corrections from @jdmpapin

hzongaro
hzongaro reviewed Jan 12, 2024
View reviewed changes
Copy link
Contributor

@hzongaro hzongaro left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, Gita, for pulling all this information together! And thanks to Devin for his thorough review!

I just have a few more minor comments.

doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
@gita-omr
Copy link
Contributor Author

Addressed latest comments from @hzongaro

@gita-omr
Copy link
Contributor Author

gita-omr commented Jan 25, 2024 •
edited
Loading

Changed all of the method and class names to use the code font.

hzongaro
hzongaro reviewed Jan 31, 2024
View reviewed changes
Copy link
Contributor

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

I think the changes all look good. Just one comment on the placeholder link added for thunk archetype.

doc/compiler/osr/OSR.md Outdated Show resolved Hide resolved
@gita-omr
Copy link
Contributor Author

Created a link to MethodHadles.md in openj9

hzongaro
hzongaro approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

Looks good! Once you've squashed the commits, I will merge.

@gita-omr
Create more detailed OSR documentation
53dc5ca 
- introduce new concepts
- provide detailed desciption of OSR mechanism based on involuntary OSR
- map new concepts into existing API
@gita-omr
Copy link
Contributor Author

gita-omr commented Feb 1, 2024

Squashed.

@hzongaro hzongaro merged commit ad592c0 into eclipse-omr:master Feb 1, 2024
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a7ehuo a7ehuo a7ehuo left review comments

@hzongaro hzongaro hzongaro approved these changes

@jdmpapin jdmpapin jdmpapin approved these changes

@charliegracie charliegracie Awaiting requested review from charliegracie

@mstoodle mstoodle Awaiting requested review from mstoodle mstoodle is a code owner

@0xdaryl 0xdaryl Awaiting requested review from 0xdaryl 0xdaryl is a code owner

Assignees
No one assigned
Labels
documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gita-omr @jdmpapin @hzongaro @a7ehuo