Skip to content

CJKFont replace InputStream.read() to byte array reading ,improve init performance.#953

Closed
oymy wants to merge 1 commit into
LibrePDF:masterfrom
oymy:cjkfont-init-optimization
Closed

CJKFont replace InputStream.read() to byte array reading ,improve init performance.#953
oymy wants to merge 1 commit into
LibrePDF:masterfrom
oymy:cjkfont-init-optimization

Conversation

@oymy

@oymy oymy commented Sep 21, 2023

Copy link
Copy Markdown

CJKFont.readCMap(String name) use InputStream.read() heavily ,which is slow.,
because the underling BufferedInputSteam does lock/unlock for every read().

Description of the new Feature/Bugfix

So load the stream into a byte[] ,then using the byte[] can avoid the costs,
leads at least 10 times faster initing CJKFont.
(with acceptable memory usage increments)

Related Issue: #

Unit-Tests for the new Feature/Bugfix

Compatibilities Issues

no

Testing details

no

…ost in BufferedInputStream.read(); 10 times faster

Signed-off-by: manyan.ouyang <manyan.ouyang@advancegroup.com>
@sonarqubecloud

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@oymy oymy changed the title replace InputStream.read() to byte array reading , to avoid locking c… CJKFont replace InputStream.read() to byte array reading , to avoid locking c… Sep 21, 2023
@oymy oymy changed the title CJKFont replace InputStream.read() to byte array reading , to avoid locking c… CJKFont replace InputStream.read() to byte array reading ,improve init performance. Sep 21, 2023
@asolntsev

Copy link
Copy Markdown
Contributor

Seems that PR #955 solves the same problem, but is shorter :)

@oymy

oymy commented Oct 17, 2023

Copy link
Copy Markdown
Author

Seems that PR #955 solves the same problem, but is shorter :)

I think my solution is better, it benefits both loading from JAR and loading from disk ( when loading from disk , the InputStream returned by the class loader is already a BufferedInputStream) .
and it reduces lock /synchronized costs that most InputSteam uses ( including BufferedInputStream).

I will do some tests to prove it. maybe in a few days.

@andreasrosdalw

andreasrosdalw commented Nov 1, 2023

Copy link
Copy Markdown
Contributor

#955 has been merged now. If evidence is shown that this is faster, then we can reconsider.

@oymy

oymy commented Nov 3, 2023

Copy link
Copy Markdown
Author

@andreasrosdal
Sorry for late response, here are my tests.

Test Explained:

I added two readCMap methods in my test:
readCMapUsingBytes (which is my solution)
readCMapUsingBufferedStream ( which is #955 solution)

Then test all three readCMap methods loading from disk /jar.

Test Results:

When loading from disk (run each methods 1000 times), the costs are:

bytes: 251
bufferedStream: 1183
old: 1151

When loading from jar (run each methos 1000 times), the costs are:

bytes: 294
bufferedStream: 631
old: 52724

Test Results Explained

When loading from disk

The underly InputStream returned from BaseFont.getResourceStream is already BufferedInputStream, so the performance of readCMapUsingBufferedStream is same as the old readCMap.
readCMapUsingBytes is faster because it reduces the cost for "synchronized" in BufferedInputStream.read

When loading from jar

readCMap is very slow, because it reads every single byte one by one from the zip file. and the costs(decoding zip but only fetch one byte) are huge (check the logic of java.util.zip.DeflaterInputStream.read).
readCMapUsingBytes is still faster than readCMapUsingBufferedStream ( still because the cost for "synchronized" )

Conclusion

My solution is faster in both loading from disk and jar. with a little more acceptable memory usage.

My Test Code

CJKFontTest.java
`package com.lowagie.text.pdf;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

import java.io.BufferedInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.time.Instant;

import static com.lowagie.text.pdf.BaseFont.RESOURCE_PATH;
import static com.lowagie.text.pdf.BaseFont.getResourceStream;
import static java.lang.Byte.toUnsignedInt;

/**

  • Created by manyan.ouyang ON 2023/11/3
    */
    class CJKFontTest {

    @test
    void test_equals() {
    String fontName = "UniJIS-UCS2-HW-V";
    Assertions.assertArrayEquals(CJKFont.readCMap(fontName), readCMapUsingBytes(fontName));
    Assertions.assertArrayEquals(CJKFont.readCMap(fontName), readCMapUsingBufferedStream(fontName));
    }

    @test
    /**

    • if open pdf is imported as jar ,then the test is performance loading from jar
      */
      void test_read_cmap_performance() {
      Instant tp0 = Instant.now();
      int cnt = 1000;
      String fontName = "UniJIS-UCS2-HW-V";
      for (int i = 0; i < cnt; i++) {
      readCMapUsingBytes(fontName);
      }

      Instant tp1 = Instant.now();
      for (int i = 0; i < cnt; i++) {
      readCMapUsingBufferedStream(fontName);
      }

      Instant tp2 = Instant.now();
      for (int i = 0; i < cnt; i++) {
      CJKFont.readCMap(fontName);
      }

      Instant tp3 = Instant.now();

      long timeCostWithBytes = (tp1.toEpochMilli() - tp0.toEpochMilli());
      long timeCostWithBufferedStream = (tp2.toEpochMilli() - tp1.toEpochMilli());
      long timeCostOld = (tp3.toEpochMilli() - tp2.toEpochMilli());

      System.out.println("bytes: " + timeCostWithBytes);
      System.out.println("bufferedStream: " + timeCostWithBufferedStream);
      System.out.println("old: " + timeCostOld);

    }

    static char[] readCMapUsingBytes(String name) {

     name = name + ".cmap";
     int size = 0x10000;
     char[] c = new char[size];
     try {
         byte[] bytes = getResourceAsBytes(RESOURCE_PATH + name, size * 2);
         for (int k = 0; k < size; ++k) {
             c[k] = (char) (((toUnsignedInt(bytes[2 * k])) << 8) + (toUnsignedInt(bytes[2 * k + 1])));
         }
         return c;
     } catch (Exception e) {
         // empty on purpose
     }
     return null;
    

    }

    static byte[] getResourceAsBytes(String key, int size) throws IOException {
    try (InputStream is = getResourceStream(key)) {
    byte[] b = new byte[size];
    is.read(b);
    return b;
    }
    }

    static char[] readCMapUsingBufferedStream(String name) {

     try {
         name = name + ".cmap";
         InputStream is = new BufferedInputStream(getResourceStream(RESOURCE_PATH + name));
         char[] c = new char[0x10000];
         for (int k = 0; k < 0x10000; ++k) {
             c[k] = (char) ((is.read() << 8) + is.read());
         }
         is.close();
         return c;
     } catch (Exception e) {
         // empty on purpose
     }
     return null;
    

    }

}`

@asolntsev

asolntsev commented Nov 3, 2023

Copy link
Copy Markdown
Contributor

@oymy Thank you for sharing the information.
Do I correctly understand that you solution basically implements the same approach as BufferedInputStream, but not synchronized? Then yes, theoretically it might work a bit faster.

Though... Java authors claim that starting from Java 6, there were optimizations added that make this difference close to zero: https://bugs.openjdk.org/browse/JDK-4097272

@oymy

oymy commented Nov 4, 2023

Copy link
Copy Markdown
Author

@oymy Thank you for sharing the information. Do I correctly understand that you solution basically implements the same approach as BufferedInputStream, but not synchronized? Then yes, theoretically it might work a bit faster.

Though... Java authors claim that starting from Java 6, there were optimizations added that make this difference close to zero: https://bugs.openjdk.org/browse/JDK-4097272

@asolntsev Thank you for the comments.

Yes. my solution is a radical version of BufferedInputStream.

I did profiling ,and the result shows that the lock/unlock costs is not close to "zero" in this scenario.

e8480ecc-14c6-4b74-9827-a1ed0cbf7068

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.

3 participants