Modified ecma string to utf8 string conversion to reduce binary size. #2214

LaszloLango · 2018-02-20T09:50:39Z

JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]

LaszloLango · 2018-02-20T09:52:18Z

Binary sizes (bytes)
6fce323:134344
c5c5db8:133852

rerobika · 2018-02-20T21:41:32Z

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

-                                         lit_get_magic_string_ex_size (string_p->u.magic_string_ex_id));
-      }
+      length = lit_utf8_string_length (lit_get_magic_string_ex_utf8 (string_p->u.magic_string_ex_id),
+                                       lit_get_magic_string_ex_size (string_p->u.magic_string_ex_id));


As I see size can be used instead of lit_get_magic_string_ex_size (string_p->u.magic_string_ex_id) like:

length = lit_utf8_string_length (lit_get_magic_string_ex_utf8 (string_p->u.magic_string_ex_id), size);

There would be one less function call.

zherczeg · 2018-02-21T09:10:45Z

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

-        length = lit_utf8_string_length (lit_get_magic_string_ex_utf8 (string_p->u.magic_string_ex_id),
-                                         lit_get_magic_string_ex_size (string_p->u.magic_string_ex_id));
-      }
+      length = lit_utf8_string_length (lit_get_magic_string_ex_utf8 (string_p->u.magic_string_ex_id), size);


I would not remove this feature. Computing the length of a longer magic string takes time, which is unnecessary if we don't care about the ascii flag.

We could use the ASCII flag as an I/O flag, if it is not set, ascii check is ignored. Otherwise flag is cleared if not ascii. Or just use another flag for requesting an ascii check.

zherczeg · 2018-02-21T09:11:06Z

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

-                                          *         false, otherwise */
+                       uint8_t *flags_p) /**< [out] flags: 0 - empty,
+                                                           1 - is ascii,
+                                                           2 - must be freed */


Is this comment is still valid?

Sorry but I don't understand your question. I've updated the comments in my patch.

flags are ECMA_STRING_FLAG_EMPTY, ... not 0, 1, ..

Just mention the enum name of these flags, or their prefix

Oh, I see. I'll fix it. Thanks

zherczeg · 2018-02-21T09:12:40Z

jerry-core/ecma/base/ecma-helpers.h

+
+#define ECMA_STRING_FLAG_EMPTY         0u
+#define ECMA_STRING_FLAG_IS_ASCII      1u
+#define ECMA_STRING_FLAG_MUST_BE_FREED 2u


I would use an enum here, with proper comments.

zherczeg · 2018-02-21T09:13:27Z

jerry-core/ecma/base/ecma-helpers.h

@@ -50,21 +50,33 @@
 */
 #define ECMA_SET_POINTER(field, non_compressed_pointer) JMEM_CP_SET_POINTER (field, non_compressed_pointer)

+bool ecma_string_to_utf8_string (const ecma_string_t *ecma_str_ptr,
+                                 const lit_utf8_byte_t **utf8_ptr,
+                                 lit_utf8_size_t *utf8_str_size);


What is this definition?

leftover, I'll remove it

LaszloLango · 2018-02-22T11:09:23Z

@zherczeg thanks for the review. I've updated the PR.

LaszloLango · 2018-02-23T08:29:45Z

@zherczeg rebased to the current master. The measurement results are still good on RPi2. There is no negative side effect of the patch.

zherczeg

Close to ready. I only have a few questions.

zherczeg · 2018-02-26T08:57:44Z

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

-    *is_ascii_p = (length == size);
+    if (length != size)
+    {
+      *flags_p &= (uint8_t) ~ECMA_STRING_FLAG_IS_ASCII;


I usually have trouble with such conversions on certain compilers.

zherczeg · 2018-02-26T08:58:44Z

jerry-core/ecma/base/ecma-helpers.h

 /**
 * Convert ecma-string's contents to a cesu-8 string and put it into a buffer.
 */
 #define ECMA_STRING_TO_UTF8_STRING(ecma_str_ptr, /**< ecma string pointer */ \
                                   utf8_ptr, /**< [out] output buffer pointer */ \
-                                   utf8_str_size) /**< [out] output buffer size */ \
+                                   utf8_str_size) /**< [in,out] output buffer size */ \


Accidental change. I'll fix it.

LaszloLango · 2018-02-26T10:43:20Z

@zherczeg Thanks for the review. I've updated the PR.

zherczeg

LGTM

JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]

dbatyai

LGTM

…jerryscript-project#2214) JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]

LaszloLango added enhancement An improvement binary size Affects binary size labels Feb 20, 2018

LaszloLango force-pushed the string-dev branch from 1953394 to 4f06255 Compare February 20, 2018 10:26

rerobika reviewed Feb 20, 2018

View reviewed changes

LaszloLango force-pushed the string-dev branch from 4f06255 to a472a56 Compare February 21, 2018 08:04

zherczeg reviewed Feb 22, 2018

View reviewed changes

LaszloLango force-pushed the string-dev branch 3 times, most recently from 2d98111 to 8ff6b1b Compare February 22, 2018 10:46

LaszloLango force-pushed the string-dev branch from 8ff6b1b to 4571ede Compare February 23, 2018 08:11

zherczeg requested changes Feb 26, 2018

View reviewed changes

LaszloLango force-pushed the string-dev branch from 4571ede to e1357b4 Compare February 26, 2018 10:42

zherczeg approved these changes Feb 26, 2018

View reviewed changes

Modified ecma string to utf8 string conversion to reduce binary size.

c5a0d1d

JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]

LaszloLango force-pushed the string-dev branch from e1357b4 to c5a0d1d Compare February 27, 2018 10:32

dbatyai approved these changes Feb 27, 2018

View reviewed changes

dbatyai merged commit e10f6d6 into jerryscript-project:master Feb 27, 2018

grgustaf pushed a commit to grgustaf/jerryscript that referenced this pull request Mar 19, 2018

Modified ecma string to utf8 string conversion to reduce binary size. (…

bfaa7c3

…jerryscript-project#2214) JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]

LaszloLango deleted the string-dev branch May 10, 2018 11:04

zherczeg mentioned this pull request Mar 19, 2019

Nominating Robert Fancsik (@rerobika) to become a JerryScript reviewer #2799

Closed

Modified ecma string to utf8 string conversion to reduce binary size. #2214

Modified ecma string to utf8 string conversion to reduce binary size. #2214

Uh oh!

Conversation

LaszloLango commented Feb 20, 2018

Uh oh!

LaszloLango commented Feb 20, 2018

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!

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!

LaszloLango commented Feb 22, 2018

Uh oh!

LaszloLango commented Feb 23, 2018

Uh oh!

zherczeg left a comment

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!

LaszloLango commented Feb 26, 2018

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

dbatyai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!