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

for fullFormat=false store plain strings in map #25

Merged
merged 2 commits into from
Dec 14, 2021
Merged

Conversation

pjfanning
Copy link
Owner

#24

@pjfanning
Copy link
Owner Author

pjfanning commented Dec 14, 2021

@mirahbo I still need to fix a broken test and add extra test coverage - when I get this working, I'll look at the temp file comments table (which you are probably not using)

@pjfanning pjfanning merged commit 854dff1 into main Dec 14, 2021
@pjfanning pjfanning deleted the perf-issue branch December 14, 2021 19:36
@pjfanning
Copy link
Owner Author

@mirahbo this is merged - the perf definitely looks much better with the changes (but only when fullFormat=false) - I can get a poi-shared-strings release done in next few days (and plugin the update to excel-streaming-reader) - if you think this change is enough, I can press on with those releases - hopefully, tomorrow

@mirahbo
Copy link

mirahbo commented Dec 14, 2021

@pjfanning I'm going to run a profiling with this new version. I think that it will meet our own requirements, because as you said, we need neither reading the comments table nor using fullFormat. But many other users will enjoy the performance boost, I guess.

@mirahbo
Copy link

mirahbo commented Dec 14, 2021

@pjfanning I did the test within our own application. I can't believe how much faster this is 🤣 The whole processing takes 20s and reading the 1 million values is not a CPU hot spot anymore 🎉

  • com.github.pjfanning.xlsx.StreamingReader$Builder.open 13.16s (65%)
  • java.util.Iterator.hasNext: 3.39s (16%) (That was the hotspot in the previous version)

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.

2 participants