Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19409003:
Update Xray for PNaCl (Closed)
|
Created:
7 years, 5 months ago by grosse Modified:
7 years, 5 months ago Reviewers:
nfullagar1 CC:
chromium-reviews, binji, Sam Clegg Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionThis modifies Xray to use the proposed PNaCl and Pepper changes, including passing strings directly to the instrumentation functions and sending the results via the Pepper Dev tracing interface.
Note: This is an experimental project within the NaCl SDK which is not normally built. NOTRY=true was added due to unrelated test failures on the trybots.
BUG=none
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212634
Patch Set 1 #Patch Set 2 : revert Earth #Patch Set 3 : add/remove files, change instrumentation ot __pnacl #
Total comments: 42
Patch Set 4 : hopefully this worked #Patch Set 5 : Change to perframe calibration #
Total comments: 9
Patch Set 6 : use TLS address for threadid #
Total comments: 18
Patch Set 7 : more style fixes #Patch Set 8 : Merge __x_profile funcs and add comments to address mask #
Total comments: 16
Patch Set 9 : miscellaneous fixes #
Total comments: 7
Patch Set 10 : Pop by depth rather than end time #Patch Set 11 : the real changes #
Total comments: 1
Messages
Total messages: 14 (0 generated)
I haven't finished reviewing this yet, but here are some comments to work on in the meantime. I also recommend updating the CL description (it looks like a broader set of changes than just the XRay ones here) See the 'Edit Issue' option in the upper left corner of the code review page) thx! https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... File native_client_sdk/src/libraries/xray/browser.c (right): https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:8: wrap this file with #ifndef XRAY_DISABLE_BROWSER_INTEGRATION https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:34: struct XrayTimestampPair XRayPepperBeginCalibration(void) { hoisted to end of line above, here and else where https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:49: struct XrayTimestampPair end_time = XRayPepperBeginCalibration(); A bit confusing above - end_time is set to 'BeginCalibration', but we've already begun calibration on the first frame. I'd also again recommend doing this calibration per-frame, some bookkeeping is already there. Consider the non-pnacl case, where XRay is using RDTSC. It might be safer to scale each frame individually and avoid wrapping the counter. (detecting the wrap would be bonus) https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:53: double scale_a = pdiff/odiff; spaces around binary operator, pdiff / odiff; (same for below and else where, scale_a * end_time...) https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:81: // arg_names[0] = (char*)(XRayTraceGetEntry(capture, i)) + 1; remove dead code? https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:91: struct XRayTraceBufferEntry* e2 = *(stack_top--); something more descriptive than 'e2' https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:108: printf("B %s %llu\n", XRayGetName(symbols, e), e->start_tick); are the printfs for debugging? imo, for browser trace report, printf should be minimal (calibration, # frames, # traces total, # annotations) https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:128: 0, 1337, why not get real TID and send it? (stash in XRayBeginFrame, uploading trace data to browser might be done from a different thread to avoid janking the application) (repeat for other calls to AddTraceEvent...) https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:140: get_browser_interface = interface; no need to stash get_browser_interface if you only need ppb_trace_interface below. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:141: trace = (PPB_Trace_Event_Dev*)get_browser_interface( ppb_trace_interface https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:145: #endif // XRAY /* XRAY */ (might as well fix report.c too) https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... File native_client_sdk/src/libraries/xray/report.c (right): https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/report.c:205: #endif // XRAY /* XRAY */ https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... File native_client_sdk/src/libraries/xray/symtable.c (right): https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/symtable.c:151: addr |= 0x10000000; macro helper for addr fixup, with a descriptive name? https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... File native_client_sdk/src/libraries/xray/xray.c (right): https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.c:97: bool start_time_initialized; wrap these as well with XRAY_DISABLE_BROWSER_INTEGRATION https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.c:106: XRAY_NO_INSTRUMENT void __pnacl_profile_func_exit(const char* fname); above: need to add back __cyg_profile versions for non-pnacl, and wrap with #if defined(__pnacl__) #else #endf https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.c:308: /* called when code is compilied with the -finstrument-functions option */ comment above should include compiler options for pnacl environ. (same for func_exit below) https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.c:309: void __pnacl_profile_func_enter(const char* fname) { fname -> this_fn, and I think you could avoid a lot of the duplicate code between pnacl & non-pnacl. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.c:397: #endif // __pnacl__ above: /* __pnacl__ */ /* c style comment */ https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.c:633: #ifndef XRAY_NOPEPPER XRAY_DISABLE_BROWSER_INTEGRATION (here & elsewhere) https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.c:635: capture->start_time = XRayPepperBeginCalibration(); XRayBrowserBeginCalibration ? https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.c:638: #endif /* XRAY_NOPEPPER */ I think the above may be better if this was done for each frame. There's already some placeholders in frame.entry[i].start_tsc & end_tsc -- these could become time stamp pair values. (See additional notes in browser.c) https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... File native_client_sdk/src/libraries/xray/xray.h (right): https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.h:102: inline void XRayBrowserTraceReport(struct XRayTraceCapture* capture); add an empty {} function body https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.h:103: inline void XRayBrowserRegisterInterface( XRayRegisterBrowserInterface( https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.h:104: PPB_GetInterface get_browser_interface); {} https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... File native_client_sdk/src/libraries/xray/xray_priv.h (right): https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray_priv.h:185: struct XrayTimestampPair { more consistent camel case - XRayTimeStampPair
https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... File native_client_sdk/src/libraries/xray/browser.c (right): https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:8: On 2013/07/17 00:53:19, nfullagar1 wrote: > wrap this file with #ifndef XRAY_DISABLE_BROWSER_INTEGRATION Done. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:34: struct XrayTimestampPair XRayPepperBeginCalibration(void) On 2013/07/17 00:53:19, nfullagar1 wrote: > { hoisted to end of line above, here and else where Done. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:81: // arg_names[0] = (char*)(XRayTraceGetEntry(capture, i)) + 1; On 2013/07/17 00:53:19, nfullagar1 wrote: > remove dead code? Done. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:145: #endif // XRAY On 2013/07/17 00:53:19, nfullagar1 wrote: > /* XRAY */ > (might as well fix report.c too) Done. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... File native_client_sdk/src/libraries/xray/report.c (right): https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/report.c:205: #endif // XRAY On 2013/07/17 00:53:19, nfullagar1 wrote: > /* XRAY */ Done. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... File native_client_sdk/src/libraries/xray/xray.c (right): https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.c:97: bool start_time_initialized; On 2013/07/17 00:53:19, nfullagar1 wrote: > wrap these as well with XRAY_DISABLE_BROWSER_INTEGRATION Done. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.c:106: XRAY_NO_INSTRUMENT void __pnacl_profile_func_exit(const char* fname); On 2013/07/17 00:53:19, nfullagar1 wrote: > above: need to add back __cyg_profile versions for non-pnacl, and wrap with #if > defined(__pnacl__) #else #endf Done. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.c:397: #endif // __pnacl__ On 2013/07/17 00:53:19, nfullagar1 wrote: > above: /* __pnacl__ */ > /* c style comment */ Done. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.c:633: #ifndef XRAY_NOPEPPER On 2013/07/17 00:53:19, nfullagar1 wrote: > XRAY_DISABLE_BROWSER_INTEGRATION (here & elsewhere) Done. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... File native_client_sdk/src/libraries/xray/xray.h (right): https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.h:102: inline void XRayBrowserTraceReport(struct XRayTraceCapture* capture); On 2013/07/17 00:53:19, nfullagar1 wrote: > add an empty {} function body Done. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.h:103: inline void XRayBrowserRegisterInterface( On 2013/07/17 00:53:19, nfullagar1 wrote: > XRayRegisterBrowserInterface( Done. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/xray.h:104: PPB_GetInterface get_browser_interface); On 2013/07/17 00:53:19, nfullagar1 wrote: > {} Done.
https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... File native_client_sdk/src/libraries/xray/browser.c (right): https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:53: double scale_a = pdiff/odiff; On 2013/07/17 00:53:19, nfullagar1 wrote: > spaces around binary operator, pdiff / odiff; > (same for below and else where, scale_a * end_time...) Done. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:91: struct XRayTraceBufferEntry* e2 = *(stack_top--); On 2013/07/17 00:53:19, nfullagar1 wrote: > something more descriptive than 'e2' Done. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:108: printf("B %s %llu\n", XRayGetName(symbols, e), e->start_tick); On 2013/07/17 00:53:19, nfullagar1 wrote: > are the printfs for debugging? imo, for browser trace report, printf should be > minimal (calibration, # frames, # traces total, # annotations) Done. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:140: get_browser_interface = interface; On 2013/07/17 00:53:19, nfullagar1 wrote: > no need to stash get_browser_interface if you only need ppb_trace_interface > below. Done. https://codereview.chromium.org/19409003/diff/4001/native_client_sdk/src/libr... native_client_sdk/src/libraries/xray/browser.c:141: trace = (PPB_Trace_Event_Dev*)get_browser_interface( On 2013/07/17 00:53:19, nfullagar1 wrote: > ppb_trace_interface Done.
getting closer! https://codereview.chromium.org/19409003/diff/21001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/browser.c (right): https://codereview.chromium.org/19409003/diff/21001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:25: static PPB_Trace_Event_Dev* trace = NULL; s/trace/ppb_trace_event_interface https://codereview.chromium.org/19409003/diff/21001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:77: if (XRayTraceIsAnnotation(capture, i)) {continue;} line above: if (...) continue; https://codereview.chromium.org/19409003/diff/21001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:80: while(*stack_top && current_tick >= (*stack_top)->end_tick){ line above: space between ) and { https://codereview.chromium.org/19409003/diff/21001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:122: trace = (PPB_Trace_Event_Dev*)interface(PPB_TRACE_EVENT_DEV_INTERFACE); ppb_trace_event_interface = ... https://codereview.chromium.org/19409003/diff/21001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/xray.c (right): https://codereview.chromium.org/19409003/diff/21001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:113: XRAY_NO_INSTRUMENT void __cyg_profile_func_exit(void* this_fn, void* call_site); plz format/indent above as it was before https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/browser.c (right): https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:73: sizeof(struct XRayTraceBufferEntry*) * (count+1)); spaces between binary ops; (count + 1) (here and elsewhere) https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:78: for(i = start; i != end; i = XRayTraceNextEntry(capture, i)) { merge two lines above: for (int i = start; ... https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:93: { remove extra scope, afaict it isn't needed. https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/symtable.c (right): https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/symtable.c:151: addr |= 0x10000000; (you may have missed this one from before) above: comment or descriptive macro to adjust addr base. https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/xray.c (right): https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:322: void __pnacl_profile_func_enter(const char* fname) { (you may have missed this one from last round) suggestion was to use this_fn instead of fname, and to merge the majority of the code between the two versions. AFAICT the code itself is the same between both, just the function name & arg list is different. https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:413: void XRayGetTSC(uint64_t* tsc) {GTSC(*tsc);} above: unless it is inlined in a class, avoid putting all on one line. ie. do this instead void foo() { statement; } https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:415: int32_t XRayGetThreadID(struct XRayTraceCapture* capture) { Need a brief comment here that ret val is the thread ID taken at BeginCapture, not the thread id of 'this' thread. https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:652: #endif /* XRAY_DISABLE_BROWSER_INTEGRATION */ code style is to have two spaces between #endif and /* https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:785: capture->thread_id = (int32_t)(&g_xray_capture); Suggest having a separate '__thread int g_xray_tid = 0; ' and a comment here that we're using the address of a TLS variable as a cheap tid value. https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/xray.h (right): https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.h:101: #include "ppapi/c/ppb.h" guarded #include "" should go up top https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/xray_priv.h (right): https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray_priv.h:186: uint64_t xray; brief comments for each field /* xray tick counter */ and /* browser timestamp */
https://codereview.chromium.org/19409003/diff/21001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/browser.c (right): https://codereview.chromium.org/19409003/diff/21001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:25: static PPB_Trace_Event_Dev* trace = NULL; On 2013/07/18 00:47:22, nfullagar1 wrote: > s/trace/ppb_trace_event_interface Done. https://codereview.chromium.org/19409003/diff/21001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:77: if (XRayTraceIsAnnotation(capture, i)) {continue;} On 2013/07/18 00:47:22, nfullagar1 wrote: > line above: > > if (...) > continue; Done. https://codereview.chromium.org/19409003/diff/21001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:80: while(*stack_top && current_tick >= (*stack_top)->end_tick){ On 2013/07/18 00:47:22, nfullagar1 wrote: > line above: space between ) and { Done. https://codereview.chromium.org/19409003/diff/21001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:122: trace = (PPB_Trace_Event_Dev*)interface(PPB_TRACE_EVENT_DEV_INTERFACE); On 2013/07/18 00:47:22, nfullagar1 wrote: > ppb_trace_event_interface = ... Done. https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/xray.c (right): https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:413: void XRayGetTSC(uint64_t* tsc) {GTSC(*tsc);} On 2013/07/18 00:47:22, nfullagar1 wrote: > above: unless it is inlined in a class, avoid putting all on one line. ie. do > this instead > > void foo() { > statement; > } Done. https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:415: int32_t XRayGetThreadID(struct XRayTraceCapture* capture) { On 2013/07/18 00:47:22, nfullagar1 wrote: > Need a brief comment here that ret val is the thread ID taken at BeginCapture, > not the thread id of 'this' thread. Done. https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:652: #endif /* XRAY_DISABLE_BROWSER_INTEGRATION */ On 2013/07/18 00:47:22, nfullagar1 wrote: > code style is to have two spaces between #endif and /* Done. https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:785: capture->thread_id = (int32_t)(&g_xray_capture); On 2013/07/18 00:47:22, nfullagar1 wrote: > Suggest having a separate '__thread int g_xray_tid = 0; ' and a comment here > that we're using the address of a TLS variable as a cheap tid value. Done. https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/xray.h (right): https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.h:101: #include "ppapi/c/ppb.h" On 2013/07/18 00:47:22, nfullagar1 wrote: > guarded #include "" should go up top Done. https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/xray_priv.h (right): https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray_priv.h:186: uint64_t xray; On 2013/07/18 00:47:22, nfullagar1 wrote: > brief comments for each field /* xray tick counter */ and /* browser timestamp > */ Done.
All the comments should be addressed now. https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/symtable.c (right): https://codereview.chromium.org/19409003/diff/24001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/symtable.c:151: addr |= 0x10000000; On 2013/07/18 00:47:22, nfullagar1 wrote: > (you may have missed this one from before) > above: comment or descriptive macro to adjust addr base. Done.
thx for addressing previous comments! a few more... https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/browser.c (right): https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:59: capture, frame, true); See comment in xray.c about XRayGetTimestamp(...) https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:74: sizeof(struct XRayTraceBufferEntry*) * (count+1)); see earlier comment (if new snapshots are uploaded, it is easy to miss earlier comments from previous snap. Alternatively, read through the comments sent via email to catch all comments across multiple snapshots) https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:79: for(i = start; i != end; i = XRayTraceNextEntry(capture, i)) { see earlier comment https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:96: { see earlier comment https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/symtable.c (right): https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/symtable.c:155: remove blank line https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/xray.c (right): https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:45: __thread int g_xray_thread_id_dummy = 0; nit: another word besides dummy https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:382: struct XRayTimestampPair* XRayGetTimestamp( Not sure I agree here returning a pointer into the capture struct. As it is used, we're not intending for the client to be able to modify these values. Returning a copy, or copying into a provided pointer would be better. Also, why not two functions, one for start, one for end. The bool isn't clear when reading browser.c With these part of a frame, it might read better as XRayFrameGetStartTimestampPair() and XRayFrameGetEndTimestampPair() https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:748: /* Use address of a thread local variable as for our fake thread id. */ above comment: re-word '...variable as for...'
https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/browser.c (right): https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:59: capture, frame, true); On 2013/07/18 18:21:06, nfullagar1 wrote: > See comment in xray.c about XRayGetTimestamp(...) Done. https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:74: sizeof(struct XRayTraceBufferEntry*) * (count+1)); On 2013/07/18 18:21:06, nfullagar1 wrote: > see earlier comment (if new snapshots are uploaded, it is easy to miss earlier > comments from previous snap. Alternatively, read through the comments sent via > email to catch all comments across multiple snapshots) Done. https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:79: for(i = start; i != end; i = XRayTraceNextEntry(capture, i)) { On 2013/07/18 18:21:06, nfullagar1 wrote: > see earlier comment The separate int i; is required for it to compile. Presumably the build scripts aren't defining -std=c99 for some reason. https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:96: { On 2013/07/18 18:21:06, nfullagar1 wrote: > see earlier comment Done. https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/symtable.c (right): https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/symtable.c:155: On 2013/07/18 18:21:06, nfullagar1 wrote: > remove blank line Done. https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/xray.c (right): https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:45: __thread int g_xray_thread_id_dummy = 0; On 2013/07/18 18:21:06, nfullagar1 wrote: > nit: another word besides dummy Done. https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:382: struct XRayTimestampPair* XRayGetTimestamp( On 2013/07/18 18:21:07, nfullagar1 wrote: > Not sure I agree here returning a pointer into the capture struct. As it is > used, we're not intending for the client to be able to modify these values. > Returning a copy, or copying into a provided pointer would be better. Also, why > not two functions, one for start, one for end. The bool isn't clear when > reading browser.c With these part of a frame, it might read better as > XRayFrameGetStartTimestampPair() and XRayFrameGetEndTimestampPair() Done. https://codereview.chromium.org/19409003/diff/36001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:748: /* Use address of a thread local variable as for our fake thread id. */ On 2013/07/18 18:21:07, nfullagar1 wrote: > above comment: re-word '...variable as for...' Done.
https://codereview.chromium.org/19409003/diff/46001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/browser.c (right): https://codereview.chromium.org/19409003/diff/46001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:74: sizeof(struct XRayTraceBufferEntry*) * (count + 1)); So XRay has a max stack depth that it will capture (XRAY_TRACE_STACK_SIZE). When surfing trace data, nothing will be deeper than that. Also, instead of comparing timestamps (which in some rare cases might go backwards if thread jumps from one physical cpu to another and their hw counters aren't in sync. We might need to protect against this and fudge the numbers if about:tracing yacks, but that could be for another CL), you could push/pop to your stack comparing to the depth field. In the ascii reporting code, the depth field is simply the number of spaces to print before printing the function name. https://codereview.chromium.org/19409003/diff/46001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:108: while(*stack_top) { hmm... it feels like this trace traversal and re-stacking could be done with one fewer while loop. This while loop looks very similar to the one above within the for loop. Is it possible to merge the two into one and make this function smaller? https://codereview.chromium.org/19409003/diff/46001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/xray.c (right): https://codereview.chromium.org/19409003/diff/46001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:69: #endif nit: there's a mix of #endif and #endif /* XRAY_DISABLE_BROWSER_INTEGRATION */ Better to have this be consistent one way or the other. https://codereview.chromium.org/19409003/diff/46001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:118: XRAY_NO_INSTRUMENT void __cyg_profile_func_exit(void* this_fn, void* call_site); above: see earlier comment re: indent as it was before.
https://codereview.chromium.org/19409003/diff/46001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/browser.c (right): https://codereview.chromium.org/19409003/diff/46001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/browser.c:74: sizeof(struct XRayTraceBufferEntry*) * (count + 1)); On 2013/07/18 21:36:00, nfullagar1 wrote: > So XRay has a max stack depth that it will capture (XRAY_TRACE_STACK_SIZE). > When surfing trace data, nothing will be deeper than that. Also, instead of > comparing timestamps (which in some rare cases might go backwards if thread > jumps from one physical cpu to another and their hw counters aren't in sync. We > might need to protect against this and fudge the numbers if about:tracing yacks, > but that could be for another CL), you could push/pop to your stack comparing to > the depth field. In the ascii reporting code, the depth field is simply the > number of spaces to print before printing the function name. Done. https://codereview.chromium.org/19409003/diff/46001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/xray.c (right): https://codereview.chromium.org/19409003/diff/46001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:69: #endif On 2013/07/18 21:36:00, nfullagar1 wrote: > nit: there's a mix of #endif and #endif /* XRAY_DISABLE_BROWSER_INTEGRATION */ > Better to have this be consistent one way or the other. Done. https://codereview.chromium.org/19409003/diff/46001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/xray.c:118: XRAY_NO_INSTRUMENT void __cyg_profile_func_exit(void* this_fn, void* call_site); On 2013/07/18 21:36:00, nfullagar1 wrote: > above: see earlier comment re: indent as it was before. Done.
lgtm :) and thanks very much for this CL! Great to see these NaCl captures showing up in about:tracing. Before checking in, please do one last round of manual testing and trace capturing, and run the final CL against the trybots before committing. thx! Besides the comment on PNACL_STRING_OFFSET, one more feature to consider for another CL is passing along the string annotations. https://codereview.chromium.org/19409003/diff/54001/native_client_sdk/src/lib... File native_client_sdk/src/libraries/xray/symtable.c (right): https://codereview.chromium.org/19409003/diff/54001/native_client_sdk/src/lib... native_client_sdk/src/libraries/xray/symtable.c:19: #define PNACL_STRING_OFFSET (0x10000000) one thought (for another CL...) is to have a known string addr at runtime to base off of instead of using a magic offset. As it is, if this offset changes we'll need to remember to change it here as well.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grosse@chromium.org/19409003/54001
Message was sent while issue was closed.
Change committed as 212634 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
