Consolidate fej9() routines

eclipse-openj9 / openj9

Eclipse OpenJ9: A Java Virtual Machine for OpenJDK that's optimized for small footprint, fast start-up, and high throughput. Builds on Eclipse OMR (https://github.com/eclipse/omr) and combines with the Extensions for OpenJDK for OpenJ9 repo.

Other

3.27k stars 721 forks source link

Consolidate fej9() routines #5259

Open dchopra001 opened 5 years ago

dchopra001 commented 5 years ago

There are multiple areas in the codebase that define

   TR_FrontEnd *fe();
   TR_J9VMBase *fej9();

Here are some examples: https://github.com/eclipse/openj9/blob/3b6cb12977ffd6e72b4ab3e9e5417317ecb4a3fe/runtime/compiler/runtime/J9CodeCacheManager.hpp#L67-L68

https://github.com/eclipse/openj9/blob/3b6cb12977ffd6e72b4ab3e9e5417317ecb4a3fe/runtime/compiler/runtime/RelocationRuntime.hpp#L135-L136

https://github.com/eclipse/openj9/blob/3b6cb12977ffd6e72b4ab3e9e5417317ecb4a3fe/runtime/compiler/codegen/J9CodeGenerator.hpp#L66

https://github.com/eclipse/openj9/blob/3b6cb12977ffd6e72b4ab3e9e5417317ecb4a3fe/runtime/compiler/env/j9method.h#L193-L194

https://github.com/eclipse/openj9/blob/3b6cb12977ffd6e72b4ab3e9e5417317ecb4a3fe/runtime/compiler/ilgen/ClassLookahead.hpp#L52-L53

https://github.com/eclipse/openj9/blob/3b6cb12977ffd6e72b4ab3e9e5417317ecb4a3fe/runtime/compiler/compile/J9Compilation.hpp#L101

These queries should be refactored into a common place. Most likely in J9Compilation.hpp.

0xdaryl commented 5 years ago

What do you mean by "a common place"? Aren't these just getters (albeit cast to different types) for the FrontEnd objects cached in these various classes? Or are you suggesting that the FE object not be cached in these classes but to dig it out of another class (such as Compilation) if it's already available there?

dchopra001 commented 5 years ago

What do you mean by "a common place"? Aren't these just getters (albeit cast to different types) for the FrontEnd objects cached in these various classes? Or are you suggesting that the FE object not be cached in these classes but to dig it out of another class (such as Compilation) if it's already available there?

I'm suggesting to get it from the Compilation class if it's already available there.

raiyansayeed commented 2 years ago

Is this issue still relevant? I would love to contribute if so.

LeahKorol commented 1 year ago

Hi, is this issue still relevant?

hzongaro commented 1 year ago

Hi, is this issue still relevant?

@LeahKorol, I took a quick look at the latest versions of the files that @dchopra001 mentioned, and it appears that it's still relevant as a clean-up item.

LeahKorol commented 1 year ago

@hzongaro , thanks for your quick answer. I'm a beginner. To ensure the idea was understood: should I define TR_FrontEnd fe();, TR_J9VMBase fej9(); in J9Compilation.hpp. and delete them from the lower files?

hzongaro commented 1 year ago

should I define TR_FrontEnd fe();, TR_J9VMBase fej9(); in J9Compilation.hpp. and delete them from the lower files?

@LeahKorol, I think those methods already exist in J9::Compilation and OMR::Compilation.

I think it would be a matter of eliminating the redundant _fe fields, and fe() and fej9() methods that exist in all of those classes, and replacing the uses of those redundant methods with uses of Compilation::fe() and Compilation::fej9() instead, assuming that a Compilation object is already available in those places (often through a comp() method).

@dchopra001, is that what you were thinking?

dchopra001 commented 1 year ago

Thank you @hzongaro for helping out here! Yes that's exactly what I was thinking.

Safest way to start here would be to look at the various classes and to remove the fej9() routines if all they're doing is grabbing the field from Compilation::fej9().

LeahKorol commented 1 year ago

Thanks! Can you please assign this to me?

LeahKorol commented 1 year ago

It was quite challenging for me to identify all the areas that need to be refactored. I don't want to make a PR as the issue isn't completely solved. I am sorry for not being able to achieve the expected results.

dchopra001 commented 1 year ago

@LeahKorol Thank you for your attempt 🙂. I understand it can be tricky to progress on this issue, especially if you are new to the codebase. If you'd like to take another shot at it, we can help you make progress and clear any roadblocks that get in your way.

Here are a few suggestions to get going if you do decide to retry:

Open a Draft Pull Request with your changes so far. We can discuss your changes there and make sure you're headed down the right path as you build out your solution.
It's not mandatory that the entire problem be solved via a single Pull Request. If the change is becoming too big we can break the problem down into smaller PRs to keep things manageable
Join the Eclipse OpenJ9 Slack here. Feel free to ask any questions in the public channels there or direct message us if you get stuck on anything.

Let us know if you have any other questions!