Conversation
|
Work in progress! build./tools/build.py --jerry-heap-profiler usagerun below code var profiler = require('profiler');
profiler.takeSnapshot('heapsnapshot.txt');heapsnapshot.txt will be generated. convert to v8 heapsnapshotnode deps/jerry/tools/j2v8snap.js heapsnapshot.txt v8.heapsnapshotsee full doc in TODO
|
| jerry_assert_api_available (); | ||
|
|
||
| #ifndef JERRY_HEAP_PROFILER | ||
| JERRY_UNUSED (path); |
There was a problem hiding this comment.
If JERRY_HEAP_PROFILER not defined, we should not do operate on any file handle.
There was a problem hiding this comment.
#define JERRY_UNUSED(x) ((void) (x))
JERRY_UNUSED does nothing on it's param. It just suppresses compiler warning "define but not used".
|
|
||
| iotjs_string_t filepath = JS_GET_ARG(0, string); | ||
| jerry_heap_profiler_take_snapshot ((const char*)iotjs_string_data(&filepath)); | ||
| return jerry_create_boolean(true); |
There was a problem hiding this comment.
Call iotjs_string_destroy(&filepath) is required before return.
src/modules/iotjs_module_profiler.c
Outdated
| return JS_CREATE_ERROR(COMMON, "filepath should be required."); | ||
|
|
||
| iotjs_string_t filepath = JS_GET_ARG(0, string); | ||
| jerry_heap_profiler_take_snapshot ((const char*)iotjs_string_data(&filepath)); |
There was a problem hiding this comment.
Remove the space after the function jerry_heap_profiler_take_snapshot, that might be only in the JerryScript source.
deps/jerry/tools/snap.js
Outdated
| @@ -0,0 +1,122 @@ | |||
| var fs = require('fs'); | |||
There was a problem hiding this comment.
Where should this file be? The CPU profiling tool are https://github.com/Rokid/ShadowNode/tree/master/tools/profiler.
There was a problem hiding this comment.
I would better place CPU and heap profiler into jerry source tree, because it is not related to shadow-node. The CPU profiler scripts will also be moved in other PR.
deps/jerry/tools/snap.js
Outdated
| const EDGE_FIELD_OFFSET_NAME = 1; | ||
| const EDGE_FIELD_OFFSET_TO_NODE = 2; | ||
|
|
||
| const DUMMY_STRING = "<dummy>"; |
There was a problem hiding this comment.
Use the single quote for strings always in JavaScript.
There was a problem hiding this comment.
3x!
The eslint has already give me many warning. I'll fix them all.
deps/jerry/tools/snap.js
Outdated
| console.log(","+JSON.stringify(stringItem.chars)); | ||
| } | ||
|
|
||
| console.log("]}"); |
There was a problem hiding this comment.
I would think here a more readable generator based on JSON is needed :)
There was a problem hiding this comment.
3x! I will learn it.
|
My understanding of strings in heap is wrong. |
…RRY_HEAP_PROFILER
yorkie
left a comment
There was a problem hiding this comment.
@algebrait leaves some considerable comments, please check and fix them :)
| ecma_extended_object_t *ext_object_p = (ecma_extended_object_t*) jmem_heap_alloc_block (size); | ||
| #ifdef JERRY_HEAP_PROFILER | ||
| ext_object_p->size = (uint32_t) size; | ||
| #endif /* JERRY_HEAP_PROFILER */ |
There was a problem hiding this comment.
The pair of macros are not alignment with the above JMEM_STATS :(
| JERRY_UNUSED (edge_type); | ||
| JERRY_UNUSED (args); | ||
|
|
||
| if (!ecma_is_value_object(to)) |
There was a problem hiding this comment.
I guess this might have a coding style issue.
| ecma_value_t value = property_pair_p->values[index].value; | ||
|
|
||
| if (ecma_is_value_object (value)) | ||
| // if (ecma_is_value_object (value)) |
There was a problem hiding this comment.
If a statement is not used, how about removing from source or add a TODO/FIXME?
| } | ||
| else | ||
| { | ||
| // TODO: when happen? |
There was a problem hiding this comment.
It's better that wrap the author name within TODO :)
// TODO(algebrait): when happen?| false, | ||
| type_error_thrower, | ||
| LIT_MAGIC_STRING_TYPE_ERROR_UL) | ||
| LIT_MAGIC_STRING_ERROR_UL) |
There was a problem hiding this comment.
This should still be LIT_MAGIC_STRING_TYPE_ERROR_UL?
| * `path` {String} profiling data file path | ||
| * Returns {null} | ||
|
|
||
| The `profiler.takeSnapshot()` methd take a Heap profiler file which path is `path`. |
There was a problem hiding this comment.
JavaScript doesn't have override, we should merge these 2 ways into:
profiler.takeSnapshot([path])
docs/devs/Optimization-Tips.md
Outdated
| $ ./tools/build.py --jerry-heap-profiler | ||
| ``` | ||
|
|
||
| Add Javascript code like docs/api/Profiler.md, Iot.js will generate a Jerry Heap Profiler file, say Profiler-123. |
There was a problem hiding this comment.
Javascript => JavaScript, and it's better that the docs/api/Profiler.md would be linkable :)
docs/devs/Optimization-Tips.md
Outdated
|
|
||
| Users can generate v8 heap snapshot file. | ||
| ```shell | ||
| node deps/jerry/tools/profiler/j2v8snap.js Profiler-123 v8.heapsnapshot |
There was a problem hiding this comment.
Add the prefix $ for shell block :p
docs/devs/Optimization-Tips.md
Outdated
| ``` | ||
|
|
||
| Then open in Chrome browser following below site. | ||
| https://developer.chrome.com/devtools/docs/heap-profiling |
src/modules/iotjs_module_profiler.c
Outdated
| jerry_value_t profiler = jerry_create_object(); | ||
| iotjs_jval_set_method(profiler, "takeSnapshot", TakeSnapshot); | ||
| iotjs_jval_set_method(profiler, "startProfiling", StartProfiling); | ||
| iotjs_jval_set_method(profiler, "stopProfiling", StopProfiling); |
There was a problem hiding this comment.
Make these strings are magic ones?
|
|
||
| /* jerry heap snapshot to V8 heap snapshot converter */ | ||
|
|
||
| let fs = require('fs'); |
|
|
||
| class SnapshotConverter { | ||
| constructor(jerrySnapshotPath, outputPath) { | ||
| let inputData = fs.readFileSync(jerrySnapshotPath, 'utf8'); |
| genNodeEdges(edges) { | ||
| for (let i = 0; i < edges.length; i++) { | ||
| let edge = edges[i]; | ||
| this.edgesOutput.push (edge.edge_type); |
There was a problem hiding this comment.
Remove the space after the function name :)
| this.edgesOutput.push (edge.edge_type); | ||
| let name; | ||
| name = this.stringMap.get(edge.name); | ||
| this.edgesOutput.push (name); |
| this.edgesOutput.push (name); | ||
| // v8 node items is a flattern array, | ||
| // so to_node is to_node_id * node_size | ||
| this.edgesOutput.push (this.nodeMap.get(edge.to) * 6); |
yorkie
left a comment
There was a problem hiding this comment.
@algebrait LGTM mostly, but still some JS coding style nits 🐰
Checklist
npm testpasses