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

Prevent cpu wastage when not logging to file #3508

Merged
merged 7 commits into from Feb 1, 2019

Conversation

@svendiedrichsen
Copy link
Contributor

svendiedrichsen commented Dec 10, 2018

  • only start pump thread if logging to file (prevents cpu wastage)
  • Store into pendingRecords queue only if logging to file (prevents server freezing when queue full)
  • draining the pending records if the pump thread gets disabled during server runtime

Fixes #3506 and payara/docker-payaraserver-full#72
and may also fix #2942
and might also be related to #2117

#3506 prevent processor wastage when not logging to file. Only start …
…pump thread if logging to file. Store into queue only if logging to file.

@svendiedrichsen svendiedrichsen changed the title #3506 prevent processor wastage when not logging to file Prevent processor wastage when not logging to file Dec 10, 2018

@svendiedrichsen svendiedrichsen changed the title Prevent processor wastage when not logging to file Prevent cpu wastage when not logging to file Dec 10, 2018

@mulderbaba mulderbaba added this to the 5.191 milestone Dec 11, 2018

@mulderbaba

This comment has been minimized.

Copy link
Member

mulderbaba commented Dec 11, 2018

jenkins test

@svendiedrichsen

This comment has been minimized.

Copy link
Contributor Author

svendiedrichsen commented Dec 11, 2018

The GFFileHandlers property flushFrequency has a default value of 1. This results in LogRecords being written and flushed one at a time. This may cause some higher resource usage when server is under load. Shouldn't this be some sensible higher number by default? At least in the example production domain?

@arjantijms arjantijms requested review from arjantijms and MarkWareham Dec 21, 2018

@rdebusscher

This comment has been minimized.

Copy link
Contributor

rdebusscher commented Jan 8, 2019

I tried this patch out and it seems that it doesn't solve the issue. CPU still at 100%. I'll investigate more.

Merge branch 'master' of https://github.com/svendiedrichsen/Payara in…
…to gffilehandler-saving-resources-not-logtofile
@svendiedrichsen

This comment has been minimized.

Copy link
Contributor Author

svendiedrichsen commented Jan 9, 2019

@rdebusscher I have just rechecked and found that if I set logtoFile to false and starting a fresh and empty domain from a freshly built server without any interaction with the server:

  • on MASTER branch cpu usage at 99% to 101%
  • on this PRs branch cpu usage 0.5% to 1.5%

Tested on an 8 core machine this shows 1 core under full load on MASTER and some rather normal load with this PR.

p.s. I have just tested 5.183 seeing the same behaviour as the current MASTER branch.

@svendiedrichsen

This comment has been minimized.

Copy link
Contributor Author

svendiedrichsen commented Jan 10, 2019

@MattGill98 The reason why this problem comes up with the payara docker image 5.184 when it did not with 5.183 is this commit where the logtoFile property is set to false on container startup. payara/docker-payaraserver-full@92e6c65

@rdebusscher

This comment has been minimized.

Copy link
Contributor

rdebusscher commented Jan 10, 2019

@svendiedrichsen Don't know what went wrong (or is it due to your last commit), but the first time I tested your patch I had issues with the following scenario. (updating a running server)

./asadmin start-domain
-> CPU low
./asadmin set-log-attributes com.sun.enterprise.server.logging.GFFileHandler.logtoFile=false
-> CPU 100%

But now that seems to be ok.

Thx for your time to have a look at this issue.

@svendiedrichsen

This comment has been minimized.

Copy link
Contributor Author

svendiedrichsen commented Jan 10, 2019

@rdebusscher You are welcome. But it is also in my best interest to see those bugs gone. ;)

(or is it due to your last commit)

This was just merging the master changes into this branch. It had worked for me before.

@MarkWareham MarkWareham removed their request for review Jan 15, 2019

@smillidge

This comment has been minimized.

Copy link
Contributor

smillidge commented Jan 26, 2019

jenkins test please

@svendiedrichsen

This comment has been minimized.

Copy link
Contributor Author

svendiedrichsen commented Jan 27, 2019

This test failed and does it irregularly with different PRs:

   [testng] FAILED: helloRemote
   [testng] javax.naming.NoInitialContextException: Cannot instantiate class: com.sun.enterprise.naming.impl.SerialInitContextFactory [Root exception is java.lang.ClassNotFoundException: com.sun.enterprise.naming.impl.SerialInitContextFactory]
   [testng] 	at javax.naming.spi.NamingManager.getInitialContext(NamingManager.java:674)
   [testng] 	at javax.naming.InitialContext.getDefaultInitCtx(InitialContext.java:313)
   [testng] 	at javax.naming.InitialContext.init(InitialContext.java:244)
   [testng] 	at javax.naming.InitialContext.<init>(InitialContext.java:216)
   [testng] 	at test.ejb.remoteview.RemoteViewTestNG.helloRemote(RemoteViewTestNG.java:59)
   [testng] Caused by: java.lang.ClassNotFoundException: com.sun.enterprise.naming.impl.SerialInitContextFactory
   [testng] 	at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
   [testng] 	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
   [testng] 	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:349)
   [testng] 	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
   [testng] 	at java.lang.Class.forName0(Native Method)
   [testng] 	at java.lang.Class.forName(Class.java:348)
   [testng] 	at com.sun.naming.internal.VersionHelper12.loadClass(VersionHelper12.java:72)
   [testng] 	at com.sun.naming.internal.VersionHelper12.loadClass(VersionHelper12.java:61)
   [testng] 	at javax.naming.spi.NamingManager.getInitialContext(NamingManager.java:672)
   [testng] 	... 26 more
   [testng] ... Removed 22 stack frames
   [testng] SKIPPED: portableGlobal
   [testng] SKIPPED: nonPortableGlobal

Seems to be something wrong with the classpath: Root exception is java.lang.ClassNotFoundException: com.sun.enterprise.naming.impl.SerialInitContextFactory

Does the classpath exceed its maximum length?

@MarkWareham

This comment has been minimized.

Copy link
Contributor

MarkWareham commented Feb 1, 2019

jenkins test please

@arjantijms arjantijms requested a review from MeroRai Feb 1, 2019

@MeroRai

MeroRai approved these changes Feb 1, 2019

@MattGill98 MattGill98 merged commit 934df64 into payara:master Feb 1, 2019

1 check passed

Payara Quick Build and Test Quick build and test passed!
Details

@svendiedrichsen svendiedrichsen deleted the svendiedrichsen:gffilehandler-saving-resources-not-logtofile branch Feb 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.