Skip to content

[COMPRESS-535] maybe we can add a break here#106

Merged
bodewig merged 1 commit intoapache:masterfrom
xenoamess-fork:should_we_add_break_here
Jun 3, 2020
Merged

[COMPRESS-535] maybe we can add a break here#106
bodewig merged 1 commit intoapache:masterfrom
xenoamess-fork:should_we_add_break_here

Conversation

@XenoAmess
Copy link
Copy Markdown
Contributor

Hi.
Can we add a break here?
Also, should we try implement a data structure or something for doing this? a loop like this sounds slow.
If you don't mind I will give it a try several hours later (if I still have some time).

Hi.
Can we add a break here?
Also, should we try implement a data structure or something for doing this? a loop like this sounds slow.
If you don't mind I will give it a try several hours later (if I still have some time).
@XenoAmess XenoAmess changed the title maybe we can add a break here [COMPRESS-535] maybe we can add a break here Jun 3, 2020
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.04%) to 87.325% when pulling 204faf5 on XenoAmess:should_we_add_break_here into f333bc6 on apache:master.

@bodewig
Copy link
Copy Markdown
Member

bodewig commented Jun 3, 2020

@XenoAmess you don't need to create a JIRA ticket for every PR. I appreciate JIRA for bigger changes, but for a single break, I'm not sure. Oh, and please add a bit more context to the PR's titles :-)

The bzip2 code has been hand optimized with an unrolled loop years ago. It is quite possible this is no longer necessary and the JIT has become smart enough to do things "right". In either case I would not go near changing the implementation without a decent performance test that proves the changed code is at least as fast as the old one used to be. For many of our users speed is more important than elegance.

@bodewig
Copy link
Copy Markdown
Member

bodewig commented Jun 3, 2020

oh, and thank you, of course

@bodewig bodewig merged commit 497cb94 into apache:master Jun 3, 2020
@XenoAmess
Copy link
Copy Markdown
Contributor Author

In either case I would not go near changing the implementation without a decent performance test that proves the changed code is at least as fast as the old one used to be.

OK, I will find some time to do the test.
I will also try build a data structure for this question, and test performance for it too.

And, maybe we should not merge this pr until the test result comes?

@bodewig
Copy link
Copy Markdown
Member

bodewig commented Jun 4, 2020

too late, I've already merged this one :-)

It is very much possible javac has become better in the fifteen years or so since the code has been written. A while ago I started https://github.com/bodewig/commons-compress-benchmarks as a limited JMH based setup to see how things changed over time. It might be a starting point - or not.

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