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

optimize the log formatter #552

Merged
merged 1 commit into from
Sep 14, 2020

Conversation

syjer
Copy link
Contributor

@syjer syjer commented Sep 12, 2020

Hi @danfickle , this PR try to optimize JDKXRLogger and especially the XRSimpleLogFormatter.

On the JDKXRLogger, we keep a Map so we don't need to go through Logger.getLogger which is kinda slow.

On the XRSimpleLogFormatter side, the biggest win is to avoid calling if necessary:

  • record.getSourceClassName
  • record.getSourceMethodName
  • printing the stacktrace

Both the record methods call internally inferCaller which it has is own complexity.

To identify which of the parameters are effectively used for the format, I went to the "ugly" route as I'm not aware of any API to check if a parameter is present or not.

I've also aligned the default from the configuration file (see Configuration.valueFor( "xr.simple-log-format") at the bottom).

Generally all this work avoid the quite heavy stacktrace walking, which could be quite long in the cases of deeply nested tables.

Additionally, I've optimized also the formatting of the "diagnostic" message. I've removed the use of String.format. See LogMessageIdFormat: this will avoid to call String.format(MESSAGE_FORMAT_PLACEHOLDER.matcher(logMessageId.getMessageFormat()).replaceAll("%s"), args); on each message formatting, which is used quite often if the user use the jdklogger.

optimize formatting of diagnostic messages
@danfickle
Copy link
Owner

Thanks, there is a lot of impressive work here (or at least it would have taken me a long time to write and debug the string parsing methods).

Merging now.

@danfickle danfickle merged commit 242a541 into danfickle:open-dev-v1 Sep 14, 2020
@syjer
Copy link
Contributor Author

syjer commented Sep 14, 2020

thank you :).

I'll have a look at others possible optimization hotspot, as I'm still wondering the reason of the issue #551 .

@testinfected
Copy link

We're having what seems to be a race condition in our tests, which run in parallel in different threads :

Caused by: java.lang.NullPointerException
	at com.openhtmltopdf.util.JDKXRLogger.getLogger(JDKXRLogger.java:103)
	at com.openhtmltopdf.util.JDKXRLogger.isLogLevelEnabled(JDKXRLogger.java:75)
	at com.openhtmltopdf.util.XRLog.log(XRLog.java:122)
	at com.openhtmltopdf.util.XRLog.log(XRLog.java:85)
	at com.openhtmltopdf.context.StyleReference.getStylesheets(StyleReference.java:253)
	at com.openhtmltopdf.context.StyleReference.setDocumentContext(StyleReference.java:102)
	at com.openhtmltopdf.pdfboxout.PdfBoxRenderer.setDocumentP(PdfBoxRenderer.java:319)
	at com.openhtmltopdf.pdfboxout.PdfBoxRenderer.setDocumentP(PdfBoxRenderer.java:292)
	at com.openhtmltopdf.pdfboxout.PdfBoxRenderer.<init>(PdfBoxRenderer.java:247)
	at com.openhtmltopdf.pdfboxout.PdfRendererBuilder.buildPdfRenderer(PdfRendererBuilder.java:67)
	at com.openhtmltopdf.pdfboxout.PdfRendererBuilder.run(PdfRendererBuilder.java:42)

I will put up a test case which reproduces the problem

@syjer
Copy link
Contributor Author

syjer commented Feb 8, 2021

hi @testinfected ,

you are totally right:

code:

public static void main(String[] args) throws Exception {

        int p = 20;
        CountDownLatch latch = new CountDownLatch(p);
        for (int i = 0; i < p;i++) {
            new Thread(() -> {
                latch.countDown();
                try {
                    latch.await();
                    XRLog.log(Level.SEVERE, LogMessageId.LogMessageId0Param.CASCADE_IS_ABSOLUTE_CSS_UNKNOWN_GIVEN);
                } catch (InterruptedException e) {
                }
            }).start();
        }
    }

is enough to reproduce the error

@syjer
Copy link
Contributor Author

syjer commented Feb 8, 2021

@testinfected I'll open a new issue so we can track it better.

@testinfected
Copy link

Thanks @syjer!

@syjer
Copy link
Contributor Author

syjer commented Feb 8, 2021

I've opened the issue #646

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants