Skip to content
This repository was archived by the owner on Mar 11, 2021. It is now read-only.

Conversation

@Therzok
Copy link
Contributor

@Therzok Therzok commented Sep 2, 2015

No description provided.

@Therzok Therzok changed the title Fix memory overhead [Do Not Merge Yet] Fix memory overhead Sep 2, 2015
@Therzok
Copy link
Contributor Author

Therzok commented Sep 2, 2015

I'll provide numbers soon and give a shout when it's ready to merge.

@Therzok
Copy link
Contributor Author

Therzok commented Sep 3, 2015

Ok, so, running the whole thing in VS diagnostics:

100 times iteration on a 65 * 1000 * 1000 bytes, gc.collect after each run - worst case possible.

  • Old version, peak usage 167mb, 14.03s runtime
    cc_old_002
  • Old version, but with memorystream initialization set - 145mb, 8.78s runtime.
    cc_init_001
  • New version as in this PR, 74mb peak memory, 5.75s runtime
    cc_byte

Without GCs:

  • Old - 259mb memory usage, 16.93s runtime
    cc2_old
  • Old with init - 351mb peak, 13.20s runtime. (one object isn't collected, so we have multiple streams alive, but it should be way lower in actual setups).
    cc2_init
  • New version - 76mb peak, 7.19s runtime.
    cc2_new

I'd definitely say this is an improvement. It also seems that the byte arrays get reused (rare collections), so this is a big +.

These numbers are just for crunching and not meant to say anything more than this solution is an improvement. Overall speed and memory usage should be improved by this PR. Marking as ready to merge.

cc @kjpou1

@Therzok Therzok changed the title [Do Not Merge Yet] Fix memory overhead Fix memory overhead Sep 3, 2015
@Therzok
Copy link
Contributor Author

Therzok commented Sep 3, 2015

Tests done with this code. File created in cmd/powershell in the working directory of the application with fsutil createnew myfile.txt 65000000.

@Therzok
Copy link
Contributor Author

Therzok commented Sep 3, 2015

Also tested them for integrity.

@Therzok
Copy link
Contributor Author

Therzok commented Sep 3, 2015

Up for theoretical case from #295:

The asset stream is X bytes.

With the old version, there would be at least ... bytes allocated:

  • X bytes for the asset stream.
  • 1 + 2 + 4 + 8 + ... + Y, where Y=2^k, Y>X
  • X bytes for the byte array.

With the init version:

  • X bytes for the asset stream.
  • X bytes for the memorystream.
  • X bytes for the bytearray.

With the new version:

  • X bytes for the asset stream.
  • X bytes for the bytearray.

@Therzok
Copy link
Contributor Author

Therzok commented Sep 3, 2015

Just a further note. It may be useful to abstract this into an internal extension method on Streams, since I've found multiple instances copyto()/toarray() calls on streams.

kjpou1 added a commit that referenced this pull request Sep 3, 2015
@kjpou1 kjpou1 merged commit 7b79fa8 into master Sep 3, 2015
@kjpou1
Copy link
Contributor

kjpou1 commented Sep 3, 2015

Thanks Marius

@Therzok Therzok deleted the fixMemoryOverhead branch September 3, 2015 10:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants