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

Fix for GC on windows #1235

Merged
merged 2 commits into from
Nov 18, 2024
Merged

Fix for GC on windows #1235

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d2577a
Fix for GC on windows
Thrameos Nov 17, 2024
c474ee8
Fix tab/spaces
Thrameos Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion native/common/include/jp_gc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,15 @@ class JPGarbageCollection
bool java_triggered;
PyObject *python_gc;
jclass _SystemClass;
jclass _ContextClass;
jmethodID _gcMethodID;

jmethodID _totalMemoryID;
jmethodID _freeMemoryID;
jmethodID _maxMemoryID;
jmethodID _usedMemoryID;
jmethodID _heapMemoryID;

size_t last_python;
size_t last_java;
size_t low_water;
Expand All @@ -69,4 +76,4 @@ class JPGarbageCollection
int python_triggered;
} ;

#endif /* JP_GC_H */
#endif /* JP_GC_H */
26 changes: 24 additions & 2 deletions native/common/jp_gc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,14 @@ void JPGarbageCollection::init(JPJavaFrame& frame)
_SystemClass = (jclass) frame.NewGlobalRef(frame.FindClass("java/lang/System"));
_gcMethodID = frame.GetStaticMethodID(_SystemClass, "gc", "()V");

jclass ctxt = frame.getContext()->m_ContextClass.get();
_ContextClass = ctxt;
_totalMemoryID = frame.GetStaticMethodID(ctxt, "getTotalMemory", "()J");
_freeMemoryID = frame.GetStaticMethodID(ctxt, "getFreeMemory", "()J");
_maxMemoryID = frame.GetStaticMethodID(ctxt, "getMaxMemory", "()J");
_usedMemoryID = frame.GetStaticMethodID(ctxt, "getUsedMemory", "()J");
_heapMemoryID = frame.GetStaticMethodID(ctxt, "getHeapMemory", "()J");

running = true;
high_water = getWorkingSize();
limit = high_water + DELTA_LIMIT;
Expand Down Expand Up @@ -237,10 +245,24 @@ void JPGarbageCollection::onEnd()
Py_ssize_t pred = current + 2 * (current - last);
last = current;
if ((Py_ssize_t) pred > (Py_ssize_t) limit)
{
run_gc = 2;
limit = high_water + (high_water>>3) + 8 * (current - last);
Copy link
Member

Choose a reason for hiding this comment

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

can you please explain this heuristic? What does it aim for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current logic is that if Python memory was rising above the high water (or predicted to) then it would trigger then java gc. When the python gc is complete we were supposed to reset the high water so that we won't run the java gc again for a while.

In Windows, it appears that rather than stop the world Python is still running. So what is happening is that in the example sent we triggered the java gc over and over and over. To prevent that I have to raise the limit above the waves so that it won't trigger unless garbage is actually piling up. Here is a graph of before and after.

image

}

// printf("consider gc %d (%ld, %ld, %ld, %ld) %ld\n", run_gc,
// current, low_water, high_water, limit, limit - pred);
#if 0
Copy link
Member

Choose a reason for hiding this comment

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

Did you forget to commit something? This is unconditional true, I think it should be activated upon instrumentation, right?

It'd also be nice to use a consistent formatting (tabs vs spaces)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The instrumentation is to barf out all of the python gc calls and all the java calls. I am not sure anyone is likely to want that unless they are trying to debug the process. So I currently have it conditionally compiled as false all the time.

I will try to fix up the tabs. The problem is that some files we have are currently tab spaced and others are spaced only and I have to keep switching the settings on vim based on which file I am editing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've found it rather painful as well while working on extension support. It got to the point where I've said to hell with it and reformatted entire files and decided to just deal with fixing it later.

Copy link
Member

Choose a reason for hiding this comment

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

We should come up with some codestyle probably. Did you choose spaces over tabs or vice versa @astrelsky?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should come up with some codestyle probably. Did you choose spaces over tabs or vice versa @astrelsky?

My ide settings were set to use this formatter so it only took me two clicks to reformat a file. https://github.com/NationalSecurityAgency/ghidra/blob/master/eclipse/GhidraEclipseFormatter.xml

I learned java from working with Ghidra so I lean towards their code style with minor divergence. In many ways it directly conflicts with the styles used in jpype. I've become very nitpicky about formatting.

To actually answer your question, I used tabs (width of 4 spaces). At the end of the day I'll follow whatever code style you set up. I was aware I was creating extra work for myself by applying the formatter when I did it.

I don't think I should have any say here. As long as it's consistent I'll follow it 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

It wouldn't be much of an effort for them to make an option like my "python setup.py build_ext --makefile" which just takes all the instructions they are running on a system and dumping it out in makefile format. I would never have made it this far in the project without it. Unfortunately with the motion towards a more opaque process it is getting harder to accomplish such development.

fwiw cmake does a pretty decent job with python extensions. The only thing that was lacking when I set it up was free threaded python support. It might be able to generate the netbeans project for you too.

Copy link
Member

Choose a reason for hiding this comment

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

I'd be in favor of tools which are independent of the used IDE. Otherwise everybody would be forced to use the same IDE, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be in favor of tools which are independent of the used IDE. Otherwise everybody would be forced to use the same IDE, right?

I'm always fighting someone to make something os/ide independent, even at work. Everyone should be able to use what they're most comfortable and productive with.

Copy link
Contributor Author

@Thrameos Thrameos Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I just want a beautifier to run from command line when we check in. So long as the style is close enough that anyone's personal IDE doesn't go insane trying process it then all is good. Double points if the beautifier is a python module so that we can just pull it in with requirements rather than installing another tool. I distributed the Netbeans IDE files in the project directory, but there is no expectation that anyone needs to use them.

(And yeah I understand IDE/build tool wars. I have developed a code base for 20 years and every new hire wants to use their IDE and a new build tool when they are going to at most touch 1% of my code base. Dropping my productivity a few precent to deal with their shiny new tool pretty much negates their value.,)

Copy link
Member

Choose a reason for hiding this comment

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

Right, so lets run pre-commit on PRs, so everybody can do whatever they love offline. When the code goes online, we enforce the formatting (this can be done automatically, but would involve rebasing to origin after a CI run).

It is also called evolution when the young ones wants to introduce state of the art stuff. I was also young one day and my bosses hated me for introducing SVN over CVS =) Sooo complicated. Several years later we had the same thingy with Git. Now everybody uses it. So after all I wouldn't say it's a bad thing to modernize once in a while.

The productivity drop is preliminary and would probably benefit in the future.

{
JPJavaFrame frame = JPJavaFrame::outer(m_Context);
jlong totalMemory = frame.CallStaticLongMethodA(_ContextClass, _totalMemoryID, nullptr);
jlong freeMemory = frame.CallStaticLongMethodA(_ContextClass, _freeMemoryID, nullptr);
jlong maxMemory = frame.CallStaticLongMethodA(_ContextClass, _maxMemoryID, nullptr);
jlong usedMemory = frame.CallStaticLongMethodA(_ContextClass, _usedMemoryID, nullptr);
jlong heapMemory = frame.CallStaticLongMethodA(_ContextClass, _heapMemoryID, nullptr);
printf("consider gc run=%d (current=%ld, low=%ld, high=%ld, limit=%ld) %ld\n", run_gc,
current, low_water, high_water, limit, limit - pred);
printf(" java total=%ld free=%ld max=%ld used=%ld heap=%ld\n", totalMemory, freeMemory, maxMemory, usedMemory, heapMemory);
}
#endif

if (run_gc > 0)
{
Expand Down
25 changes: 25 additions & 0 deletions native/java/org/jpype/JPypeContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,4 +637,29 @@ private static void scanExistingJars()
}
}

private static long getTotalMemory()
{
return Runtime.getRuntime().totalMemory();
}

private static long getFreeMemory()
{
return Runtime.getRuntime().freeMemory();
}

private static long getMaxMemory()
{
return Runtime.getRuntime().maxMemory();
}

private static long getUsedMemory()
{
return Runtime.getRuntime().totalMemory() - Runtime.getRuntime().freeMemory();
}

private static long getHeapMemory()
{
java.lang.management.MemoryMXBean memoryBean = java.lang.management.ManagementFactory.getMemoryMXBean();
return memoryBean.getHeapMemoryUsage().getUsed();
}
}
Loading