Skip to content

Refactor literal-storage to not use C++ features #791

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

Closed

Conversation

rtakacs
Copy link
Contributor

@rtakacs rtakacs commented Dec 22, 2015

This patch removes all C++ features from the literal-storage. The functionality is the same, just the code structure is different.

Measurement results:

testing board:  rpi2
master revision: 4f44071
section master size with patch
.note.gnu.build-id 36 36
.text 190012 189612
.rodata 8176 8124
.ARM.exidx 8 8
.init_array 4 4
.data 1096 1096
.bss 2363840 2363848
.comment 40 40
.ARM.attributes 47 17
Total 2563259 2562815

@zherczeg
Copy link
Member

Nice job!

@seanshpark
Copy link
Contributor

+1

@rtakacs rtakacs force-pushed the refactor-lit-storage branch from 46efb89 to 5219470 Compare December 23, 2015 10:39
@sand1k
Copy link
Contributor

sand1k commented Dec 24, 2015

I've rebased this patch onto the latest master. Please, check https://github.com/Samsung/jerryscript/tree/refactor-lit-storage-rebased

@rtakacs rtakacs force-pushed the refactor-lit-storage branch from 5219470 to 72c64df Compare December 29, 2015 18:53
@rtakacs
Copy link
Contributor Author

rtakacs commented Dec 29, 2015

Thank you Andrey! I've updated this PR.

@rtakacs
Copy link
Contributor Author

rtakacs commented Dec 29, 2015

Measurement results:

testing board:  rpi2
master revision: 50d124b
section size (master) size (patch)
.note.gnu.build-id 36 36
.text 191100 191052
.rodata 8344 8332
.ARM.exidx 8 8
.init_array 4 4
.data 1096 1096
.bss 2363776 2363784
.comment 40 40
.ARM.attributes 47 47
Total 2564451 2564399
Benchmark RSS
(+ is better)
Perf
(+ is better)
3d-cube.js 112-> 112 (0.000) 2.979-> 2.98 (-0.034)
access-binary-trees.js 84-> 84 (0.000) 1.835-> 1.833 (0.109)
access-fannkuch.js 40-> 40 (0.000) 8.816-> 8.98 (-1.860)
access-nbody.js 48-> 48 (0.000) 4.15-> 4.206 (-1.349)
bitops-3bit-bits-in-byte.js 32-> 32 (0.000) 2.255-> 2.261 (-0.266)
bitops-bits-in-byte.js 32-> 32 (0.000) 3.041-> 3.019 (0.723)
bitops-bitwise-and.js 32-> 32 (0.000) 3.415-> 3.411 (0.117)
controlflow-recursive.js 220-> 220 (0.000) 1.545-> 1.555 (-0.647)
crypto-aes.js 124-> 124 (0.000) 5.478-> 5.33 (2.702)
crypto-md5.js 184-> 184 (0.000) 33.818->32.642 (3.477)
crypto-sha1.js 132-> 132 (0.000) 14.971->14.542 (2.865)
date-format-xparb.js 72-> 72 (0.000) 1.656-> 1.655 (0.060)
math-cordic.js 40-> 40 (0.000) 3.29-> 3.305 (-0.456)
math-partial-sums.js 32-> 32 (0.000) 1.909-> 1.889 (1.048)
math-spectral-norm.js 40-> 40 (0.000) 2.057-> 2.047 (0.486)
string-fasta.js 48-> 48 (0.000) 5.662-> 5.588 (1.307)
Geometric mean: RSS reduction: 0% Speed up: 0.528%

* string */
record_t *
create_charset_record (record_set_t *rec_set_p, /**< recordset */
const lit_utf8_byte_t *str, /**< string to be placed in the record */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

  1. str -> str_p
  2. imho 'placed in the record' -> 'placed into the record'

extern bool rcs_record_is_equal (rcs_record_set_t *, rcs_record_t *, rcs_record_t *);
extern bool rcs_record_is_equal_utf8 (rcs_record_set_t *, rcs_record_t *, const lit_utf8_byte_t *, lit_utf8_size_t);

#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, we put comments after #endif to describe condition, corresponding to code just above the #endif. For example, !RCS_RECORDS_H in the case.

@ruben-ayrapetyan
Copy link
Contributor

Looks good to me

@rtakacs rtakacs force-pushed the refactor-lit-storage branch 3 times, most recently from c94f0a7 to 00379d6 Compare January 12, 2016 12:14
#endif /* JERRY_ENABLE_SNAPSHOT */
extern rcs_record_t *lit_create_charset_literal (rcs_record_set_t *, const lit_utf8_byte_t *, lit_utf8_size_t);
extern rcs_record_t *lit_create_magic_literal (rcs_record_set_t *, lit_magic_string_id_t);
extern rcs_record_t *lit_create_magic_record_ex (rcs_record_set_t *, lit_magic_string_ex_id_t);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lit_create_magic_literal_ex

@rtakacs rtakacs force-pushed the refactor-lit-storage branch from 00379d6 to f138345 Compare January 12, 2016 13:40
uint32_t *);
#endif /* JERRY_ENABLE_SNAPSHOT */

#endif /* RCS_SNAPSHOT_H */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be !RCS_SNAPSHOT_H ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it should, I missed that.

@rtakacs rtakacs force-pushed the refactor-lit-storage branch from f138345 to 9dec857 Compare January 12, 2016 14:09
JERRY_ASSERT (RCS_RECORD_IS_MAGIC_STR_EX (rec_p));

rcs_record_set_field (rec_p, RCS_MAGIC_STR_HEADER_ID_POS, RCS_MAGIC_STR_HEADER_ID_WIDTH, id);
} /* rcs_record_set_magic_str_id */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be: rcs_record_set_magic_str_ex_id

JerryScript-DCO-1.0-Signed-off-by: Roland Takacs [email protected]
@rtakacs rtakacs force-pushed the refactor-lit-storage branch from 9dec857 to a997b59 Compare January 13, 2016 07:50
@galpeter
Copy link
Contributor

lgtm

@galpeter
Copy link
Contributor

merged: 21f561f

@jiangzidong
Copy link
Contributor

@galpeter @sand1k and @ruben-ayrapetyan
Is Jerry going to remove all c++ features and become a pure C project?

@tilmannOSG
Copy link

@jiangzidong Yes, that's correct. Several months ago a decision was made to move back to C and we are now incrementally rewriting the remaining C++ parts of JerryScript in C.

@jiangzidong
Copy link
Contributor

@tilmannOSG Thanks. Do you have a schedule about when to complete the refactor

@tilmannOSG
Copy link

@jiangzidong We don't have a strict schedule but we aim to complete the move to C in the coming months.

@jiangzidong
Copy link
Contributor

@tilmannOSG I see, and I am working on removing c++ features of the rcs_chunked_list

@tilmannOSG
Copy link

@jiangzidong That's great :) Contributions in that area are very welcome!

@rtakacs rtakacs deleted the refactor-lit-storage branch January 18, 2019 14:29
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.

9 participants