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

Improve performance of ASM's ClassReader.readStream() #26665

Closed
wants to merge 1 commit into from

Conversation

stsypanov
Copy link
Contributor

@stsypanov stsypanov commented Mar 11, 2021

Hello, ClassReader.readStream() can be reworked to improve its performance and reduce memory consumption. Currently there are two major problems about it:

  • we always allocate a buffer of the same size which is often several times bigger than actual amount of data being read from InputStream
  • we always return of copy of byte[] underlying in ByteArrayOutputStream

Using InputStream.available() we can pre-size the buffer in vast majority of cases and even when the whole data is not read within one iteration (for large InputStreams) we allocate as much memory as required.

Another optimization is about checking whether it was only one read from InputStream, and in this case the buffer itself can be returned instead of calling ByteArrayOutputStream.toByteArray().

I've used this benchmark available here to check impact of the patch for start-up of simple Spring Boot application:

@State(Scope.Thread)
@BenchmarkMode(Mode.SingleShotTime)
@OutputTimeUnit(TimeUnit.MILLISECONDS)
@Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
public class SpringBootApplicationBenchmark {

    private ConfigurableApplicationContext context;

    @Benchmark
    public Object startUp() {
        return context = SpringApplication.run(BenchmarkApplication.class);
    }

    @TearDown(Level.Invocation)
    public void close() {
        context.close();
    }
}

and got the following results:

original

Benchmark                                                   Mode  Cnt         Score       Error   Units
SpringBootApplicationBenchmark.startUp                        ss  200       718.760 ±    20.549   ms/op
SpringBootApplicationBenchmark.startUp:·gc.alloc.rate         ss  200        38.895 ±     0.620  MB/sec
SpringBootApplicationBenchmark.startUp:·gc.alloc.rate.norm    ss  200  49776228.720 ± 19919.815    B/op
SpringBootApplicationBenchmark.startUp:·gc.count              ss  200           ≈ 0              counts

patched

Benchmark                                                   Mode  Cnt         Score       Error   Units
SpringBootApplicationBenchmark.startUp                        ss  200       614.820 ±     7.031   ms/op
SpringBootApplicationBenchmark.startUp:·gc.alloc.rate         ss  200        41.678 ±     0.240  MB/sec
SpringBootApplicationBenchmark.startUp:·gc.alloc.rate.norm    ss  200  49003403.920 ± 21786.877    B/op
SpringBootApplicationBenchmark.startUp:·gc.count              ss  200           ≈ 0              counts

@sbrannen
Copy link
Member

The following is our general stance on modification of the repackaged ASM code.

Please refrain from modifying classes under org.springframework.asm, org.springframework.cglib, and org.springframework.objenesis. Those include repackaged forks of the third-party libraries ASM, CGLIB, and Objenesis. Any refactoring to those classes should take place upstream in the originating repository. The Spring Framework will then pick up the changes when syncing with official updates of the forked third-party libraries.

In any case, let's see if @jhoeller wishes to patch our repackaged ASM code with your proposal.

@sbrannen sbrannen requested a review from jhoeller March 11, 2021 11:19
@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement labels Mar 11, 2021
@sbrannen sbrannen changed the title Improve performance of ClassReader.readStream() Improve performance of ASM's ClassReader.readStream() Mar 11, 2021
@stsypanov
Copy link
Contributor Author

@sbrannen Sorry, I totally forgot that org.springframework.asm is 3rd-party code

@sbrannen
Copy link
Member

@sbrannen Sorry, I totally forgot that org.springframework.asm is 3rd-party code

No problem.

I was about to point you to the ASM repository, but I see that you just created this PR there 17 minutes ago. 😉

@jhoeller, in light of that we can wait on that PR to get merged into ASM proper and then consume it later.

@sbrannen
Copy link
Member

Closing in favor of the upstream PR against ASM: https://gitlab.ow2.org/asm/asm/-/merge_requests/314

@sbrannen sbrannen closed this Mar 11, 2021
@sbrannen sbrannen removed the request for review from jhoeller March 11, 2021 12:45
@sbrannen sbrannen added for: external-project Needs a fix in external project and removed status: waiting-for-triage An issue we've not yet triaged or decided on in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement labels Mar 11, 2021
@stsypanov stsypanov deleted the cr branch March 11, 2021 14:10
@stsypanov stsypanov restored the cr branch March 13, 2021 21:10
@stsypanov stsypanov deleted the cr branch April 19, 2021 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: external-project Needs a fix in external project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants