Move snapshot generation function to the snapshot tool #2057

galpeter · 2017-10-25T17:26:10Z

Now there is a snapshot tool which can merge snapshots
but was unable to generate snapshots by itself.

This change moves the snapshot generation utilities
to the snapshot tool.

JerryScript-DCO-1.0-Signed-off-by: Peter Gal pgal.u-szeged@partner.samsung.com

akosthekiss · 2017-10-25T22:25:30Z

jerry-main/main-unix-snapshot.c

+  fwrite (output_buffer, sizeof (uint8_t), snapshot_size, snapshot_file_p);
+  fclose (snapshot_file_p);
+
+  printf ("Created snapshot file: '%s' (%d bytes)\n", output_file_name_p, snapshot_size);


cppcheck signals signed/unsigned int mismatch here ("warning(invalidPrintfArgType_sint): %d in format string (no. 2) requires a signed integer given in the argument list.")

akosthekiss · 2017-10-25T22:25:58Z

jerry-main/main-unix-snapshot.c

+    fwrite (output_buffer, sizeof (uint8_t), literal_buffer_size, literal_file_p);
+    fclose (literal_file_p);
+
+    printf ("Created literal file: '%s' (%d bytes)\n", save_literals_file_name_p, literal_buffer_size);


... and here

zherczeg · 2017-10-26T10:13:57Z

I like this change

galpeter · 2017-10-27T10:01:06Z

@akosthekiss I've updated the PR.

akosthekiss · 2017-10-27T14:49:08Z

jerry-main/main-unix-snapshot.c

  return bytes_read;
 } /* read_file */

+/** Generate command line option IDs


this comment text seems to be off by one line

akosthekiss · 2017-10-27T14:53:55Z

jerry-main/main-unix-snapshot.c

+               .help = "print generated opcodes"),
+  CLI_OPT_DEF (.id = OPT_GENERATE_CONTEXT, .opt = "c", .longopt = "context",
+               .help = "specify the execution context of the snapshot: "
+                       "global or eval (default: global).", .meta = "MODE"),


a bit unidiomatic placement of the .meta initializer (it used to follow .opt/.longopt because that's how it is printed, too)

akosthekiss · 2017-10-27T14:55:40Z

jerry-main/main-unix-snapshot.c

+  uint32_t number_of_files = 0;
+  uint8_t *source_p = input_buffer;
+  size_t source_length = 0;
+  const char *output_file_name_p = "js.snapshot";


there should be one single default output file name for all snapshot sub-commands (generate, merge, ...)

IMHO having different default output names makes the intent more clear.
But we can change it to a common one. Currently merge uses "merged.snapshot", so what do you think would be the best common name?

"js.snapshot" is fine by me for both. it'd be a quite simple and elegant default.

akosthekiss · 2017-10-27T16:04:19Z

jerry-main/main-unix-snapshot.c

+               .help = "export literals found in parsed JS input (in list format)"),
+  CLI_OPT_DEF (.id = OPT_GENERATE_LITERAL_C, .longopt = "save-literals-c-format",
+               .meta = "FILE",
+               .help = "export literals found in parsed JS input (in C source format)"),


the original --save-snapshot-for-{global,eval} options have already been rewritten to a new --context={global,eval} format. wouldn't it make sense to have something similar for --save-literals-{c,list}-format, too? e.g., as simple as --save-literals={c,list}.

the logic is different, for --save-literals* you need to specify the output file as argument. This way you can generate the literal file and also the snapshot.

oh that's right

akosthekiss · 2017-10-27T16:20:10Z

tools/run-tests.py

+def update_test_args_with_snapshot_tool(jerry_path, test_args):
+    if "--snapshot-tool" in test_args:
+        snapshot_idx = test_args.index("--snapshot-tool")
+        test_args[snapshot_idx] = "--snapshot-tool=%s-snapshot" % (jerry_path)


is this rewrite really needed? patching up the cli options this way is a bit creepy, and the same effect could be achieved by simply adding this "deduce snapshot tool path from engine path" logic to the run-test-suite.sh script. (explanation following there...)

akosthekiss · 2017-10-27T16:23:59Z

tools/runners/run-test-suite.sh

-    shift
+
+    SNAPSHOT_TOOL=${1#--snapshot-tool=}
+    SNAPSHOT_TOOL=(${SNAPSHOT_TOOL//,/ })


...following from above: you'd just have SNAPSHOT_TOOL=$ENGINE-snapshot here. (agreed, it looses a bit of flexibility by not allowing for snapshot tools placed anywhere in the file system for those who run this script manually. but who does get restricted by that, actually? run-test.py does force to have jerry and jerry-snapshot side-by-side anyway)

Certainly we can simply assume the path, but I wanted to give the test site runner more freedom if it is executed manually.

But I'll update this part.

akosthekiss

minor changes requested

akosthekiss · 2017-10-30T18:36:33Z

tools/runners/run-test-suite.sh

+        echo "$0: $SNAPSHOT_TOOL: not an executable"
+        exit 1
+    fi
+    shift;


no need for the semicolon (shouldn't be used elsewhere either)

semicolon still here but removed from after IS_SNAPSHOT=true?

ping on this minor change request

akosthekiss · 2017-10-30T18:43:43Z

jerry-main/main-unix-snapshot.c

+
+  if (save_literals_file_name_p != NULL)
+  {
+    const size_t literal_buffer_size = jerry_parse_and_save_literals ((jerry_char_t *) source_p,


seems that the arg indentation is off by one space

Now there is a snapshot tool which can merge snapshots but was unable to generate snapshots by itself. This change moves the snapshot generation utilities to the snapshot tool. JerryScript-DCO-1.0-Signed-off-by: Peter Gal pgal.u-szeged@partner.samsung.com

akosthekiss

LGTM

Note: I've experimented with GitHub's feature of allowing maintainer changes. So, I've fixed the semicolon issue and pushed it to the PR branch. Seems to work.

zherczeg

LGTM

galpeter added infrastructure Related to GH Actions or the tested targets snapshot Related to the snapshot feature tools Related to the tooling scripts labels Oct 25, 2017

akosthekiss reviewed Oct 25, 2017

View reviewed changes

galpeter force-pushed the snapshot-tool-generate branch from b161996 to 2e166b0 Compare October 27, 2017 10:00

galpeter force-pushed the snapshot-tool-generate branch 4 times, most recently from e8995a6 to 3939c7b Compare October 27, 2017 13:08

akosthekiss requested changes Oct 27, 2017

View reviewed changes

galpeter force-pushed the snapshot-tool-generate branch from 3939c7b to 018d2e3 Compare October 30, 2017 15:07

akosthekiss requested changes Oct 30, 2017

View reviewed changes

galpeter force-pushed the snapshot-tool-generate branch from 018d2e3 to c6abc91 Compare October 31, 2017 11:17

akosthekiss force-pushed the snapshot-tool-generate branch from c6abc91 to 7e3d805 Compare November 4, 2017 18:52

akosthekiss approved these changes Nov 4, 2017

View reviewed changes

zherczeg approved these changes Nov 5, 2017

View reviewed changes

akosthekiss merged commit 8ae6592 into jerryscript-project:master Nov 5, 2017

Move snapshot generation function to the snapshot tool #2057

Move snapshot generation function to the snapshot tool #2057

Uh oh!

Conversation

galpeter commented Oct 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Oct 26, 2017

Uh oh!

galpeter commented Oct 27, 2017

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!

akosthekiss 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!

Choose a reason for hiding this comment

Uh oh!

akosthekiss left a comment

Choose a reason for hiding this comment

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants