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

Use non-autoflush PrintStream rather than LogStream for mall prices. #1388

Merged
merged 2 commits into from Dec 31, 2022

Conversation

phulin
Copy link
Contributor

@phulin phulin commented Dec 30, 2022

I meant to include this in #1382 - the price writing code should use a buffered output stream, instead of a LogStream which auto-flushes each byte array write.

This PR reduces time on the below benchmark from about 10 s to about 500 ms.

  @Test
  void speed() {
    int N = 300;
    MallPriceDatabase.savePricesToFile = true;
    for (var entry : ItemDatabase.entrySet()) {
      MallPriceDatabase.recordPrice(entry.getKey(), entry.getKey(), true);
    }
    for (int i = 0; i < 2 * N / 5; i++) {
      MallPriceDatabase.writePrices();
    }
    long start =
        ManagementFactory.getThreadMXBean().getThreadCpuTime(Thread.currentThread().getId());
    for (int i = 0; i < N; i++) {
      MallPriceDatabase.writePrices();
    }
    long end = ManagementFactory.getThreadMXBean().getThreadCpuTime(Thread.currentThread().getId());
    System.out.println("Elapsed: " + (end - start) / 1_000_000);
    new File(KoLConstants.DATA_LOCATION, "mallprices.txt").delete();
  }

@phulin phulin requested a review from a team as a code owner December 30, 2022 21:16
@codecov
Copy link

codecov bot commented Dec 30, 2022

Codecov Report

Merging #1388 (91b90e9) into main (d24fa65) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head 91b90e9 differs from pull request most recent head a2eb298. Consider uploading reports for the commit a2eb298 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1388      +/-   ##
============================================
- Coverage     33.56%   33.55%   -0.01%     
+ Complexity    16747    16746       -1     
============================================
  Files          1049     1049              
  Lines        162125   162128       +3     
  Branches      34849    34850       +1     
============================================
- Hits          54412    54408       -4     
- Misses        98381    98387       +6     
- Partials       9332     9333       +1     
Impacted Files Coverage Δ
...eforge/kolmafia/persistence/MallPriceDatabase.java 41.61% <0.00%> (-0.29%) ⬇️
.../sourceforge/kolmafia/request/StandardRequest.java 81.08% <0.00%> (-2.71%) ⬇️
src/net/sourceforge/kolmafia/RequestThread.java 36.96% <0.00%> (-1.22%) ⬇️
...ourceforge/kolmafia/textui/command/BuyCommand.java 7.50% <0.00%> (-0.40%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d24fa65...a2eb298. Read the comment docs.

Copy link
Member

@midgleyc midgleyc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmation: does the writer flush automatically before being closed?

This is probably a problem in many places in the codebase; could it be centralised somewhere? Doesn't have to be done as part of this PR. I think we have many classes that deal with reading from / writing to files already.

@phulin
Copy link
Contributor Author

phulin commented Dec 31, 2022

It does flush before writing, see PrintStream.close documentation:

image

@phulin
Copy link
Contributor Author

phulin commented Dec 31, 2022

And yeah, it should be done elsewhere. DataUtilities.getOutputStream should probably return a buffered output stream by default...

@midgleyc midgleyc enabled auto-merge (squash) December 31, 2022 14:24
@midgleyc midgleyc merged commit e692b14 into kolmafia:main Dec 31, 2022
@midgleyc midgleyc added the performance Improve Mafia speed / CPU label Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Improve Mafia speed / CPU
Projects
None yet
2 participants