Optimize string character access for ascii strings #971

zherczeg · 2016-03-18T10:13:03Z

Optimize string character access for ascii strings and refactor type store to allow two byte hashes in the future.

LaszloLango · 2016-03-18T10:28:01Z

jerry-core/ecma/base/ecma-helpers-string.c

+  return ecma_number_to_utf8_string (*num_p, buffer, sizeof (buffer));
+} /* ecma_string_get_heap_number_size */
+
+#define ECMA_STRING_IS_ASCII(char_p, size) ((size) == lit_utf8_string_length ((char_p), (size)))


Missing comment

zherczeg · 2016-03-18T11:18:21Z

                                         Benchmark |                   Perf(+ is better)
                                        3d-cube.js |   2.565s ->   2.605s :  -1.559%  (+-1.623%) : [≈]
                            access-binary-trees.js |   1.357s ->   1.345s :  +0.921%  (+-1.637%) : [≈]
                                access-fannkuch.js |   7.580s ->   7.723s :  -1.880%  (+-1.238%) : [-]
                                   access-nbody.js |   2.940s ->   2.940s :  +0.000%  (+-1.147%) : [≈]
                       bitops-3bit-bits-in-byte.js |   1.675s ->   1.712s :  -2.239%  (+-1.349%) : [-]
                            bitops-bits-in-byte.js |   2.455s ->   2.482s :  -1.120%  (+-0.914%) : [-]
                             bitops-bitwise-and.js |   3.285s ->   3.265s :  +0.609%  (+-2.171%) : [≈]
                             bitops-nsieve-bits.js |  20.165s ->  20.230s :  -0.322%  (+-1.766%) : [≈]
                          controlflow-recursive.js |   0.938s ->   0.943s :  -0.533%  (+-2.209%) : [≈]
                                     crypto-aes.js |   3.540s ->   3.502s :  +1.059%  (+-0.790%) : [+]
                                     crypto-md5.js |   7.180s ->   3.583s : +50.105%  (+-0.804%) : [+]
                                    crypto-sha1.js |   3.885s ->   2.208s : +43.179%  (+-0.761%) : [+]
                              date-format-tofte.js |   2.178s ->   2.130s :  +2.181%  (+-0.656%) : [+]
                              date-format-xparb.js |   0.860s ->   0.840s :  +2.326%  (+-0.000%) : [+]
                                    math-cordic.js |   2.880s ->   2.925s :  -1.562%  (+-1.326%) : [-]
                              math-partial-sums.js |   1.580s ->   1.613s :  -2.057%  (+-0.924%) : [-]
                             math-spectral-norm.js |   1.448s ->   1.485s :  -2.591%  (+-1.558%) : [-]
                                  string-base64.js |  30.968s ->   5.105s : +83.515%  (+-0.239%) : [+]
                                   string-fasta.js |   3.328s ->   3.270s :  +1.728%  (+-2.047%) : [≈]
                                   Geometric mean: |                  Speed up: 14.673% (+-0.330%) : [+]

LaszloLango · 2016-03-18T12:00:24Z

jerry-core/ecma/base/ecma-helpers-string.c

+  bool is_ascii;
+  const lit_utf8_byte_t *chars_p = ecma_string_raw_chars (string_p, &buffer_size, &is_ascii);
+
+  if (chars_p != NULL)


I can't see how could be the chars_p NULL. Could you show me an example? :)

I found out. (ECMA_STRING_CONTAINER_UINT32_IN_DESC and ECMA_STRING_CONTAINER_HEAP_NUMBER has no raw string data)

zherczeg · 2016-03-19T08:05:38Z

Please review the patch again.

akosthekiss · 2016-03-19T08:38:51Z

jerry-core/ecma/base/ecma-helpers-string.c

+ * The ecma string ref and container fields should fill the 16 bit field.
+ */
+JERRY_STATIC_ASSERT ((ECMA_STRING_MAX_REF | ECMA_STRING_CONTAINER_MASK) == UINT16_MAX,
+                     ecma_string_ref_and_container_fields_should_fill_the_16_bit_field);


I think one more static assert is missing: the one that checks that ECMA_STRING_CONTAINER_MASK is bigger than or equal to the largest possible value of ecma_string_container_t.

Shall I add a new enum for max testing? I wouldn't want to overdo this.

A new enum is certainly not needed. But you can add a new label to the end of the existing ecma_string_container_t to get the count of labels automatically calculated. I've seen that used in some other enums as well (and called something like xxx__COUNT).

Unfortunately the side effect is that I need to extend switches with this new value (doesn't look too nice). Do you still want it?

I still would like to see an assert and also an automatically calculated __COUNT label for the enum. But that does not necessarily require the extension of switches.

I've looked for the switches with ECMA_STRING_CONTAINER_ case labels and almost all of them already had default branches with some kind of an assert (unreachable, or checking for an explicitly given container type), which is what they should do IMHO. The only exception I've found is in function ecma_deref_ecma_string, which does not check for invalid values -- but it should (or already should have) IMHO!

Where have you found switches that needed extension?

Added a commit which adds the changes.
I don't really like the double JERRY_UNREACHABLE() sequences.

zherczeg · 2016-03-21T06:08:27Z

Patch updated with a new approach.

LaszloLango · 2016-03-21T08:06:26Z

LGTM, but please update the copyright headers in

jerry-core/ecma/base/ecma-globals.h
jerry-core/ecma/operations/ecma-string-object.c
jerry-core/lit/lit-magic-strings.inc.h

akosthekiss · 2016-03-21T08:18:32Z

@zherczeg With the latest revision, you loose an important feature of the __COUNT pattern: it's used in C projects to automatically count the valid enums. This helps keeping the code safe, as however and whoever edits the enum (adds or removes labels), the count is always updated and its value is always correct. So, it's more then just a style pattern. The manually maintained __MAX approach looses these benefits. (Also, it goes against the style of the project, which uses the __COUNT approach all around, but not a __MAX terminology.)

Regarding the default branches: they are not only to signal that "there are other valid enums, but they cannot appear here because of certain reasons". They are also safeguards to take care of the case when the data format (i.e., the list of valid labels in an enum) has been extended but someone forgot to update the code that works with it. (Basically, whenever you have a switch, it's a very good practice to also have a default branch, or to have explicitly documented why it's not needed.) Even existing string container code uses switch with a default branch and unreachable assert -- even though it covers all valid cases (somewhere around string copies). Moreover, if you only have an unreachable assert macro in the default branch, it turns to a no-op in release code, so it has no effect on code size or running time there.

zherczeg · 2016-03-21T08:35:57Z

"someone forgot to update the code that works with it" -> this is my biggest fear. Updating a max is easy, since it is around the typedef, and whoever works with it will notice it. However, default branches eliminate compiler warnings and will create hidden errors which the testing system might not capture (or a lot of debugging needed). Personally I like full (all values checked) switches without defaults. Anyway if you insist I will do your request even if I don't agree at all.

zherczeg · 2016-03-21T09:02:52Z

I changed the __MAX to be part of the enum, and it works.

akosthekiss · 2016-03-21T09:05:30Z

LGTM (FWIW)

…store to allow two byte hashes in the future. JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com

zherczeg · 2016-03-21T09:32:23Z

Copyright updated.

LaszloLango added enhancement An improvement performance Affects performance ecma core Related to core ECMA functionality labels Mar 18, 2016

LaszloLango added this to the Engine optimization & enhancement milestone Mar 18, 2016

LaszloLango reviewed Mar 18, 2016
View reviewed changes

zherczeg force-pushed the ascii_string_optimization branch 2 times, most recently from 6cf4065 to a5143e1 Compare March 19, 2016 08:04

akosthekiss reviewed Mar 19, 2016
View reviewed changes

zherczeg force-pushed the ascii_string_optimization branch from 68d7253 to 218d8c2 Compare March 21, 2016 06:07

zherczeg force-pushed the ascii_string_optimization branch from 218d8c2 to 5911f72 Compare March 21, 2016 09:02

Optimize string character access for ascii strings and refactor type …

af24694

…store to allow two byte hashes in the future. JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com

zherczeg force-pushed the ascii_string_optimization branch from 5911f72 to af24694 Compare March 21, 2016 09:32

zherczeg merged commit af24694 into jerryscript-project:master Mar 21, 2016

zherczeg deleted the ascii_string_optimization branch March 21, 2016 09:33

zherczeg mentioned this pull request Apr 7, 2016

Nominating Akos Kiss (akiss77) for JerryScript Maintainer status #992

Closed

Optimize string character access for ascii strings #971

Optimize string character access for ascii strings #971

Uh oh!

Conversation

zherczeg commented Mar 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Mar 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Mar 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Mar 21, 2016

Uh oh!

LaszloLango commented Mar 21, 2016

Uh oh!

akosthekiss commented Mar 21, 2016

Uh oh!

zherczeg commented Mar 21, 2016

Uh oh!

zherczeg commented Mar 21, 2016

Uh oh!

akosthekiss commented Mar 21, 2016

Uh oh!

zherczeg commented Mar 21, 2016

Uh oh!

Uh oh!