Skip to content

Simplify string management. #1189

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

Merged
merged 1 commit into from
Jul 8, 2016

Conversation

zherczeg
Copy link
Member

@zherczeg zherczeg commented Jul 6, 2016

Allocate a single memory block for strings, rather than a separate string header
and string characters block. In the past strings were split into 8 byte chunks,
and large amount of legacy code is designed for that representation. However the
current allocator allows block allocation so we don't need those complicated
algorithms anymore. This patch is a cleanup rather than an optimization.

Small speedup is achieved though. Binary size unchanged.

Benchmark Perf (sec)
3d-cube.js 0.950 -> 0.955 : -0.573%
3d-raytrace.js 1.113 -> 1.120 : -0.587%
access-binary-trees.js 0.586 -> 0.585 : +0.221%
access-fannkuch.js 2.568 -> 2.575 : -0.276%
access-nbody.js 1.075 -> 1.070 : +0.476%
bitops-3bit-bits-in-byte.js 0.571 -> 0.575 : -0.792%
bitops-bits-in-byte.js 0.849 -> 0.861 : -1.359%
bitops-bitwise-and.js 1.199 -> 1.107 : +7.667%
bitops-nsieve-bits.js 1.820 -> 1.849 : -1.578%
controlflow-recursive.js 0.402 -> 0.394 : +1.882%
crypto-aes.js 1.149 -> 1.168 : -1.711%
crypto-md5.js 0.753 -> 0.747 : +0.757%
crypto-sha1.js 0.709 -> 0.703 : +0.852%
date-format-tofte.js 0.886 -> 0.871 : +1.733%
date-format-xparb.js 0.482 -> 0.466 : +3.213%
math-cordic.js 1.352 -> 1.341 : +0.865%
math-partial-sums.js 0.747 -> 0.724 : +3.168%
math-spectral-norm.js 0.595 -> 0.589 : +1.132%
string-base64.js 2.088 -> 2.082 : +0.259%
string-fasta.js 1.858 -> 1.831 : +1.443%
Geometric mean: +0.862%

Allocate a single memory block for strings, rather than a separate string header
and string characters block. In the past strings were split into 8 byte chunks,
and large amount of legacy code is designed for that representation. However the
current allocator allows block allocation so we don't need those complicated
algorithms anymore. This patch is a cleanup rather than an optimization.

JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com
@LaszloLango LaszloLango added enhancement An improvement performance Affects performance labels Jul 6, 2016
@LaszloLango
Copy link
Contributor

@zherczeg, any effect on memory usage?

@zherczeg
Copy link
Member Author

zherczeg commented Jul 6, 2016

No, because non-ascii strings are not tested by sunspider.

This patch also a preparation for allowing 32 bit compressed (actually uncompressed) pointers as well.

@zherczeg
Copy link
Member Author

zherczeg commented Jul 6, 2016

The good thing is the effect of the patch is slightly positive on all string manipulation tests. However since the slow cases are improved mostly, the effect is small (as they should!).

@tilmannOSG
Copy link

Nice cleanup! I'm not familiar enough with the string handling code to provide substantial feedback. Great to see that this will bring us a step closer to offering the option of uncompressed pointers as well :)

@jiangzidong
Copy link
Contributor

Hi @zherczeg , after the patch, some kinds of strings (ECMA_STRING_CONTAINER_HEAP_UTF8_STRING) are not managed by pool chunk (jmem_pools_alloc), and not counted in the related pool_stats. Right?

@zherczeg
Copy link
Member Author

zherczeg commented Jul 7, 2016

Yes. I consider the pool allocator as a legacy system, which should be moved out from the project eventually.

@jiangzidong
Copy link
Contributor

@zherczeg so in the future, every ecma-type will be directly managed by heap, without the poolman,, right?

@zherczeg
Copy link
Member Author

zherczeg commented Jul 7, 2016

Ideally yes. The current allocator is capable of allocating blocks with any length. The old allocator allocated 64 byte blocks, so it wasted too much space without this pool system. This is not a problem anymore.

@jiangzidong
Copy link
Contributor

@zherczeg Thanks. I didn't notice that the allocating blocks have no length constraints. Cool!

@LaszloLango
Copy link
Contributor

LGTM

1 similar comment
@dbatyai
Copy link
Member

dbatyai commented Jul 8, 2016

LGTM

@zherczeg zherczeg merged commit 97be8bf into jerryscript-project:master Jul 8, 2016
@zherczeg zherczeg deleted the string_rework branch July 8, 2016 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement performance Affects performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants