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

Excessive use of j9file_lastmod from shared class cache #84

Open
DanGritter opened this issue Sep 20, 2017 · 5 comments
Open

Excessive use of j9file_lastmod from shared class cache #84

DanGritter opened this issue Sep 20, 2017 · 5 comments

Comments

@DanGritter
Copy link

So what I believe is happening is there are two pairs of places that can trigger a notifyClasspathEntryStateChange through j9shr_hookZipLoadEvent. One pair is through the JVM_ZipHook out of Java_java_util_zip_ZipFile_close / Java_java_util_zip_ZipFile_open (handle zip file access from Java), and the other pair is from the J9HOOK_VM_ZIP_LOAD event registered in shrinit.cpp, which receives notification from TRIGGER_J9HOOK_VM_ZIP_LOAD, used by the zip cache library used by the zip file access from native.

So even though a file is open and stays open in the zip cache by native, a Java application can open and close a duplicate zip file, breaking the state of the timestamp cache used by hasTimestampChanged() as it doesn't keep track of open count.

Essentially we get something like:

open(xml.jar) from J9HOOK_VM_ZIP_LOAD
open(xml.jar) from JVM_ZipHook
close(xml.jar) from JVM_ZipHook

After this point hasTimestampChanged(xml.jar) fails to identify xml.jar as an already open file and continues to invoke the localCheckTimestamp() routine, driving j9file_lastmod.

@hangshao0
Copy link
Contributor

Yes. It looks like we need to record the open count.

@hangshao0
Copy link
Contributor

hangshao0 commented Sep 25, 2017

We could add a field to record the open count in class CpLinkedListHdr.
Increase this count by 1 if notifyClasspathEntryStateChange() is called with J9ZIP_STATE_OPEN. Decrease it by 1 if notifyClasspathEntryStateChange() is called with J9ZIP_STATE_CLOSED. Flag CPM_ZIP_OPEN should only be removed when open count is decreased to 0.

@pshipton
Copy link
Member

The information needed is if the classloader is holding open the jar file, and therefore the contents cannot change. On Unix platforms, the file could still change although something is holding the jar file open. I think the counting isn't a robust solution (although it would work on Windows).

@pshipton pshipton added enhancement and removed bug labels Sep 28, 2017
@pshipton pshipton added this to Java 8 bringup in Issue tracking Oct 25, 2017
@pshipton pshipton removed this from Java 8 bringup in Issue tracking Oct 25, 2017
@pshipton
Copy link
Member

pshipton commented Dec 7, 2017

Shared classes uses j9file_lastmod() to check jar file timestamps, which takes a filename parameter. Using a fd instead of a filename may resolve this issue. The shared cache doesn't currently keep fds open for the jar files, but perhaps it could do so during startup and other periods of heavy class loading. It would be easy to close fds after startup by using the J9VM_PHASE_NOT_STARTUP phase change.

It is possible to check timestamps via the fd rather than the filename (https://linux.die.net/man/2/fstat, https://msdn.microsoft.com/en-us/library/windows/desktop/ms724320(v=vs.85).aspx).

Using j9file_lastmod() is also a problem on Windows. In a startup test of Liberty it was found timestamp checking on Windows costs about 2.4%. On Windows using j9file_lastmod() involves converting the filename from utf8 to unicode, which is the likely slowdown.

@DanHeidinga
Copy link
Member

@hangshao0 Is this something you're looking into?

ebadyano pushed a commit to ebadyano/openj9 that referenced this issue Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants