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

Consume less memory #159

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Consume less memory #159

wants to merge 13 commits into from

Conversation

Miha-x64
Copy link
Contributor

@Miha-x64 Miha-x64 commented Mar 1, 2018

Reduced memory consumption:
— removed unused BitInteger constants in IterImpl;
— changed IterImplNumber.intDigits, IterImplNumber.floatDigits type from int[] to byte[];
— changed IterImplString.hexDigits type from int[] to byte[] and shifted it by 48 ('0');
— using byte constants instead of ValueType in JsonIterator, using own byte[] dictionary instead of ValueType[].

@codecov-io
Copy link

codecov-io commented Mar 1, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@72231fa). Click here to learn what that means.
The diff coverage is 67.94%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #159   +/-   ##
========================================
  Coverage          ?   68.5%           
========================================
  Files             ?     107           
  Lines             ?    7354           
  Branches          ?    1388           
========================================
  Hits              ?    5038           
  Misses            ?    1869           
  Partials          ?     447
Impacted Files Coverage Δ
...ain/java/com/jsoniter/ReflectionObjectDecoder.java 50.18% <ø> (ø)
...rc/main/java/com/jsoniter/extra/Base64Support.java 78.57% <ø> (ø)
.../main/java/com/jsoniter/output/CodegenImplMap.java 95.08% <ø> (ø)
src/main/java/com/jsoniter/IterImpl.java 65.65% <ø> (ø)
src/main/java/com/jsoniter/IterImplString.java 87.5% <0%> (ø)
src/main/java/com/jsoniter/IterImplNumber.java 67.44% <0%> (ø)
src/main/java/com/jsoniter/any/ArrayLazyAny.java 78.04% <100%> (ø)
src/main/java/com/jsoniter/JsonIteratorPool.java 72.72% <100%> (ø)
src/main/java/com/jsoniter/any/LazyAny.java 45.16% <100%> (ø)
.../main/java/com/jsoniter/output/MapKeyEncoders.java 84.21% <50%> (ø)
... and 4 more

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 72231fa...f8e8764. Read the comment docs.

@taowen
Copy link
Contributor

taowen commented Mar 2, 2018

what is your usage scenario to concern the memory usage so much?

@Miha-x64
Copy link
Contributor Author

Miha-x64 commented Mar 2, 2018

I can't see any reason to bloat memory. When we can use 256 bytes instead of 1K, why don't do so?

@taowen
Copy link
Contributor

taowen commented Mar 2, 2018

Do you have memory benchmark to prove this change worthwhile? It looks non substantial to me.

@Miha-x64
Copy link
Contributor Author

Miha-x64 commented Mar 2, 2018

public static void main(String[] args) throws Exception {
    JsonIterator ji = new JsonIterator();
    ji.reset("[1, \"str\"]".getBytes());

    IterImpl.nextToken(ji);
    System.out.println(IterImplNumber.readInt(ji));
    IterImpl.nextToken(ji);
    System.out.println(IterImplString.readString(ji));

    System.gc();
    System.gc();
    System.gc();

    System.out.println(Runtime.getRuntime().freeMemory());
    /* -Xmx4M, running from IDEA
     *
     * Java version | 1.8.0_131 | 9         |
     * -------------|-----------|-----------|
     * Original     | 3 230 040 | 2 956 408 |
     * Optimized    | 3 254 040 | 2 979 504 |
     *              |      Free memory      |
     * _____________|_______________________|
     */
}

Output differs a bit between invocations. In general, I've saved about 20 KB.
It's not a thing, but i don't see any reason to allocate more, e. g. by using int[] in places where byte[] is appropriate.

@taowen
Copy link
Contributor

taowen commented Mar 2, 2018

the saving is not proportional to the length of input. 20kb constant memory saving is not worth it.

@Miha-x64
Copy link
Contributor Author

Miha-x64 commented Mar 3, 2018

I's a bit philosophical decision: when I can free some memory without affecting performance, I do it.
https://www.youtube.com/watch?v=b6zKBZcg5fk

@miere
Copy link

miere commented Apr 18, 2019

@Miha-x64 Totally agreed. Specially if we use jsoniter where we have strict hardware constraints, like AWS Lambda functions.

@plokhotnyuk
Copy link
Contributor

I would say that this PR proposes a trade off which save memory by spending more CPU cycles in runtime.

Also, worst cases in case of malformed input are getting more worse here. As example rethowing of exception can take the same time as a small message parsing. See more info here: https://shipilev.net/blog/2014/exceptional-performance/

@miere
Copy link

miere commented Apr 18, 2019 via email

@miere
Copy link

miere commented Apr 18, 2019 via email

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.

None yet

5 participants