Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15732012:
Revive voronoi multi-threaded demo. (Closed)
|
Created:
7 years, 7 months ago by nfullagar1 Modified:
7 years, 6 months ago CC:
chromium-reviews, binji, Sam Clegg Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionRevive voronoi multi-threaded demo.
BUG=none
TEST=demo for SDK
R=binji@chromium.org, noelallen@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202991
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 76
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #
Messages
Total messages: 11 (0 generated)
Still looking, but you should remove these extra files. https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... File native_client_sdk/src/examples/demo/voronoi/common.js (right): https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... native_client_sdk/src/examples/demo/voronoi/common.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Why does this have it's own common? https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... File native_client_sdk/src/examples/demo/voronoi/example.dsc (right): https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... native_client_sdk/src/examples/demo/voronoi/example.dsc:20: 'example.js', Remove https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... File native_client_sdk/src/examples/demo/voronoi/example.js (right): https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... native_client_sdk/src/examples/demo/voronoi/example.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Remove https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... File native_client_sdk/src/examples/demo/voronoi/index.html (right): https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... native_client_sdk/src/examples/demo/voronoi/index.html:17: data-tools="newlib glibc pnacl linux" data-configs="Debug Release" win? https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... File native_client_sdk/src/examples/demo/voronoi/make.bat (right): https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... native_client_sdk/src/examples/demo/voronoi/make.bat:1: @..\..\tools\make.exe %* Remove, auto generated on windows
removed files https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... File native_client_sdk/src/examples/demo/voronoi/common.js (right): https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... native_client_sdk/src/examples/demo/voronoi/common.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/05/24 18:42:50, noelallen1 wrote: > Why does this have it's own common? removed https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... File native_client_sdk/src/examples/demo/voronoi/example.dsc (right): https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... native_client_sdk/src/examples/demo/voronoi/example.dsc:20: 'example.js', On 2013/05/24 18:42:50, noelallen1 wrote: > Remove See new version of example.js https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... File native_client_sdk/src/examples/demo/voronoi/example.js (right): https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... native_client_sdk/src/examples/demo/voronoi/example.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/05/24 18:42:50, noelallen1 wrote: > Remove see new version https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... File native_client_sdk/src/examples/demo/voronoi/make.bat (right): https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... native_client_sdk/src/examples/demo/voronoi/make.bat:1: @..\..\tools\make.exe %* On 2013/05/24 18:42:50, noelallen1 wrote: > Remove, auto generated on windows removed
https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... File native_client_sdk/src/examples/demo/voronoi/index.html (right): https://codereview.chromium.org/15732012/diff/1/native_client_sdk/src/example... native_client_sdk/src/examples/demo/voronoi/index.html:17: data-tools="newlib glibc pnacl linux" data-configs="Debug Release" On 2013/05/24 18:42:50, noelallen1 wrote: > win? The thread_pool class currently uses semaphore.h - if win supports the same flavor, I can add it back. (or see the TODO on converting thread pool to use condvar instead of sem)
drive by review! https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/example.dsc (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.dsc:12: 'CXXFLAGS': [ shouldn't be necessary, we already optimize release builds https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/example.js (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.js:7: function moduleDidLoad() { we usually use attachListeners() for this. See the nacl_io example https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.js:8: document.getElementById("benchmark").addEventListener("click", single quotes for strings https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.js:11: alert("Please wait while running benchmark.") alert() doesn't work in packaged apps. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.js:12: }, false); ", false" shouldn't be needed. We don't need to capture events from child elements here. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.js:13: document.getElementById("draw_points").addEventListener("click", we use camelCase for ids https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.js:29: document.getElementById("one").addEventListener("click", this could probably be made a lot more obvious by name the elements radio1, radio2, etc. and looping over the list: threads = [1,2,4,...]; for (var i = 0; i < threads.length; ++i) { // this anonymous function is necessary to capture the current value of i, or all closures will use the last value of i. (function (i) { document.getElementById('radio'+i).addEventListener('click', ...) })(i); } https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.js:77: var x = Math.round(message_event.data * 1000) / 1000; You can just use parseFloat(message_event.data) to convert from a string, and rely on toFixed to round it. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/index.html (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/index.html:11: <title>Multi-threaded Voronoi Demo</title> {{title}} https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/index.html:16: data-name="voronoi" data-width="512" data-height="512" all of the data params (except width and height) are filled out by the template. Just use {{attrs}} https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/index.html:19: <h1>Voronoi</h1> {{title}} https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/index.html:24: <br /> no need to close these elements. <br> or <input> is fine. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/threadpool.h (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.h:36: pthread_t *threads_; might want to run clang-format on this diff to fix * issues, etc. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.h:48: for(int i = 0; i < num_tasks; i++) work(i, data); } share with SingleThread? https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.h:49: explicit ThreadPool(int num_threads) { ; } nit: {} https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/voronoi.cc (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:30: const int kStartRecurseSize = 32; must be power of 2 https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:73: Vec2() { ; } init to 0? https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:215: // (not really cones - since it is all realative, we avoid doing the sp: relative https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:219: int n = 0; maybe a better var name here... closest_index? https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:220: float zz = kHugeZ; maybe minz? https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:241: assert(parts * parts == num_regions_); num_regions should be const too, I think. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:372: wRenderRect(x, y, w, h); assert that w, h are power of two? https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:377: void Voronoi::wRenderRegionEntry(int region, void *thiz) { I've been calling this a "thunk" https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:501: point_count_ = num_points < kMaxPointCount ? num_points : kMaxPointCount; check for negative? https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:535: const size_t bytes = size.width() * size.height(); unused https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:560: // The Module class. The browser calls the CreateInstance() method to create kill comment https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:574: // Factory function called by the browser when the module is first loaded. kill comment
WRT Thread Pool: 1- Should be legal to pass 0-N as the number of threads. 0 means work is done on the thread calling dispatch. N means works is done on the one or more threads in the pool. 2- Thread pool seems to be combination of ThreadPool and WorkQueue. The thread pool assumes that knowing which worker number you are is enough. Would it make more sense to have your main thread enqueue work which would be say structure containing the corners of the cell instead of the job number? You could then have a seperate function for 'Done' giving someone the opportunity to add work, then go away, and check if it's done at a later time. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/threadpool.cc (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.cc:19: ThreadPool::ThreadPool(const int num_threads) const? Wasn't this non-const in your header? Maybe assign num_threads to num_threads_ in the const int constructor? https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.cc:22: if (num_threads_ > 1) { Shouldn't a thread pool of 1 work? https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.cc:146: // SingleThread() will dispatch all work on this thread. You already check for >1 elsewhere, why not make this one function? https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.cc:148: for (int i = 0; i < num_tasks; i++) Use of "single thread" is confusing. In the case that ThreadPool is constructed with only a single thread, that thread would never be used. So either you create 2-N threads which will do all the work outside of your main thread, or you create no threads and must call SingleThread. There is no case for create 1 thread to do the work off your main thread. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/threadpool.h (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.h:23: #if !defined(DISABLE_THREADS) I would recommend removing. We either should support this everywhere or nowhere IMO. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.h:38: int num_threads_; Did you mean to make this one const? https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.h:49: explicit ThreadPool(int num_threads) { ; } A bit strange that in the DISABLE_THREADS case the .cc is empty. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/voronoi.cc (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:73: Vec2() { ; } We typically use {} not {;} https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:203: RequestInputEvents(PP_INPUTEVENT_CLASS_MOUSE); You aren't looking at mouse events.
ptal - hopefully I've addressed most comments. ThreadPool had a small re-factor, instead of posting and waiting across num_tasks, it posts and waits across num_threads to reduce the number of semaphore calls. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/example.dsc (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.dsc:12: 'CXXFLAGS': [ On 2013/05/24 21:47:55, binji wrote: > shouldn't be necessary, we already optimize release builds (background) added to speed up debug build, for benchmarking. I'll remove it. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/example.js (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.js:7: function moduleDidLoad() { On 2013/05/24 21:47:55, binji wrote: > we usually use attachListeners() for this. See the nacl_io example Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.js:8: document.getElementById("benchmark").addEventListener("click", On 2013/05/24 21:47:55, binji wrote: > single quotes for strings Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.js:11: alert("Please wait while running benchmark.") On 2013/05/24 21:47:55, binji wrote: > alert() doesn't work in packaged apps. Ok, changed to update Status: while benchmarking https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.js:12: }, false); On 2013/05/24 21:47:55, binji wrote: > ", false" shouldn't be needed. We don't need to capture events from child > elements here. Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.js:13: document.getElementById("draw_points").addEventListener("click", On 2013/05/24 21:47:55, binji wrote: > we use camelCase for ids Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.js:29: document.getElementById("one").addEventListener("click", On 2013/05/24 21:47:55, binji wrote: > this could probably be made a lot more obvious by name the elements radio1, > radio2, etc. and looping over the list: > > threads = [1,2,4,...]; > for (var i = 0; i < threads.length; ++i) { > // this anonymous function is necessary to capture the current value of i, or > all closures will use the last value of i. > (function (i) { > document.getElementById('radio'+i).addEventListener('click', ...) > })(i); > } Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.js:77: var x = Math.round(message_event.data * 1000) / 1000; On 2013/05/24 21:47:55, binji wrote: > You can just use parseFloat(message_event.data) to convert from a string, and > rely on toFixed to round it. This arrives as a float, and we're rounding to 3 digits. (toFixed doesn't round.) https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/index.html (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/index.html:11: <title>Multi-threaded Voronoi Demo</title> On 2013/05/24 21:47:55, binji wrote: > {{title}} Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/index.html:16: data-name="voronoi" data-width="512" data-height="512" On 2013/05/24 21:47:55, binji wrote: > all of the data params (except width and height) are filled out by the template. > Just use {{attrs}} Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/index.html:19: <h1>Voronoi</h1> On 2013/05/24 21:47:55, binji wrote: > {{title}} Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/index.html:24: <br /> On 2013/05/24 21:47:55, binji wrote: > no need to close these elements. <br> or <input> is fine. Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/threadpool.cc (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.cc:19: ThreadPool::ThreadPool(const int num_threads) On 2013/05/25 00:21:11, noelallen1 wrote: > const? Wasn't this non-const in your header? Maybe assign num_threads to > num_threads_ in the const int constructor? Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.cc:22: if (num_threads_ > 1) { On 2013/05/25 00:21:11, noelallen1 wrote: > Shouldn't a thread pool of 1 work? changed so num_threads_ of 0 does all tasks on dispatch thread. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.cc:146: // SingleThread() will dispatch all work on this thread. On 2013/05/25 00:21:11, noelallen1 wrote: > You already check for >1 elsewhere, why not make this one function? see other changes https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.cc:148: for (int i = 0; i < num_tasks; i++) On 2013/05/25 00:21:11, noelallen1 wrote: > Use of "single thread" is confusing. In the case that ThreadPool is > constructed with only a single thread, that thread would never be used. So > either you create 2-N threads which will do all the work outside of your main > thread, or you create no threads and must call SingleThread. There is no case > for create 1 thread to do the work off your main thread. changed so 0 = dispatch thread, 1 = one thread off dispatch thread https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/threadpool.h (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.h:23: #if !defined(DISABLE_THREADS) On 2013/05/25 00:21:11, noelallen1 wrote: > I would recommend removing. We either should support this everywhere or > nowhere IMO. removed https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.h:36: pthread_t *threads_; On 2013/05/24 21:47:55, binji wrote: > might want to run clang-format on this diff to fix * issues, etc. Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.h:38: int num_threads_; On 2013/05/25 00:21:11, noelallen1 wrote: > Did you mean to make this one const? yes! thx. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.h:48: for(int i = 0; i < num_tasks; i++) work(i, data); } On 2013/05/24 21:47:55, binji wrote: > share with SingleThread? Fishing this out and sharing this one line of code is more loc... https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.h:49: explicit ThreadPool(int num_threads) { ; } On 2013/05/24 21:47:55, binji wrote: > nit: {} Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/threadpool.h:49: explicit ThreadPool(int num_threads) { ; } On 2013/05/25 00:21:11, noelallen1 wrote: > A bit strange that in the DISABLE_THREADS case the .cc is empty. DISABLE_THREADS removed https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/voronoi.cc (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:30: const int kStartRecurseSize = 32; On 2013/05/24 21:47:55, binji wrote: > must be power of 2 comment added https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:73: Vec2() { ; } On 2013/05/24 21:47:55, binji wrote: > init to 0? This struct is closer to an int or a float (in my mind) which aren't set to zero by default. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:73: Vec2() { ; } On 2013/05/25 00:21:11, noelallen1 wrote: > We typically use {} not {;} Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:203: RequestInputEvents(PP_INPUTEVENT_CLASS_MOUSE); On 2013/05/25 00:21:11, noelallen1 wrote: > You aren't looking at mouse events. Not now. The thought being if a user makes local changes to handle mouse events, they won't be filtered by default. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:215: // (not really cones - since it is all realative, we avoid doing the On 2013/05/24 21:47:55, binji wrote: > sp: relative Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:219: int n = 0; On 2013/05/24 21:47:55, binji wrote: > maybe a better var name here... closest_index? s/n/closest_cell, removed comment on return statement https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:220: float zz = kHugeZ; On 2013/05/24 21:47:55, binji wrote: > maybe minz? Is comment above sufficient? https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:241: assert(parts * parts == num_regions_); On 2013/05/24 21:47:55, binji wrote: > num_regions should be const too, I think. Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:372: wRenderRect(x, y, w, h); On 2013/05/24 21:47:55, binji wrote: > assert that w, h are power of two? done (but assert is down in CreateContext) https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:377: void Voronoi::wRenderRegionEntry(int region, void *thiz) { On 2013/05/24 21:47:55, binji wrote: > I've been calling this a "thunk" hmm... if you don't feel strongly about this, it feels better (imo) for the Dispatch() function to invoke an Entry function as opposed to a Thunk function. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:501: point_count_ = num_points < kMaxPointCount ? num_points : kMaxPointCount; On 2013/05/24 21:47:55, binji wrote: > check for negative? Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:535: const size_t bytes = size.width() * size.height(); On 2013/05/24 21:47:55, binji wrote: > unused good catch https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:560: // The Module class. The browser calls the CreateInstance() method to create On 2013/05/24 21:47:55, binji wrote: > kill comment Done. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:574: // Factory function called by the browser when the module is first loaded. On 2013/05/24 21:47:55, binji wrote: > kill comment Done.
lgtm https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/example.js (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.js:77: var x = Math.round(message_event.data * 1000) / 1000; On 2013/05/28 23:11:50, nfullagar1 wrote: > On 2013/05/24 21:47:55, binji wrote: > > You can just use parseFloat(message_event.data) to convert from a string, and > > rely on toFixed to round it. > This arrives as a float, and we're rounding to 3 digits. (toFixed doesn't > round.) Seems to from my tests in the Chrome console: (3.0045).toFixed(3) "3.005" (3.0044).toFixed(3) "3.004" https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/voronoi.cc (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:241: assert(parts * parts == num_regions_); On 2013/05/28 23:11:50, nfullagar1 wrote: > On 2013/05/24 21:47:55, binji wrote: > > num_regions should be const too, I think. > > Done. Ah, I just meant it seemed strange to have parts defined here, but have it rely on num_regions_ being a specific value which is defined in Voronoi -- I see it is because Render uses it below. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/voronoi.cc:377: void Voronoi::wRenderRegionEntry(int region, void *thiz) { On 2013/05/28 23:11:50, nfullagar1 wrote: > On 2013/05/24 21:47:55, binji wrote: > > I've been calling this a "thunk" > hmm... if you don't feel strongly about this, it feels better (imo) for the > Dispatch() function to invoke an Entry function as opposed to a Thunk function. Nope, Entry is fine.
One last nit:
In threadpool the use of Dispatch, SingleThread, Multhread is still a bit
confusing. This is really just a naming thing.
Perhaps something like;
Dispatch->ProcessWork
SingleThread->ProcessNow || ProcessLocally
MultiThread->Dispatch
ProcessWork() {
if (numthreads) {
Dispatch(....)
} else {
ProcessNow(...)
}
}
Anyway, the single thread when theadnum =0 instead of 1 and the term dispatch
when actually processing in place is a bit strange. Otherwise LGTM
re: Noel's naming concern, discussed offline and changed MultiThread() & SingleThread() to DispatchMany() & DispatchHere(). re: toFixed(), see reply. https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... File native_client_sdk/src/examples/demo/voronoi/example.js (right): https://codereview.chromium.org/15732012/diff/10001/native_client_sdk/src/exa... native_client_sdk/src/examples/demo/voronoi/example.js:77: var x = Math.round(message_event.data * 1000) / 1000; On 2013/05/28 23:37:50, binji wrote: > On 2013/05/28 23:11:50, nfullagar1 wrote: > > On 2013/05/24 21:47:55, binji wrote: > > > You can just use parseFloat(message_event.data) to convert from a string, > and > > > rely on toFixed to round it. > > This arrives as a float, and we're rounding to 3 digits. (toFixed doesn't > > round.) > > Seems to from my tests in the Chrome console: > (3.0045).toFixed(3) > "3.005" > (3.0044).toFixed(3) > "3.004" I should have said toFixed doesn't round 'as expected.' :) The Stackoverflow articles indicate toFixed is inconsistent regarding rounding across platforms, and the oft suggested workaround is to use var roundedNum = (Math.round( num * 1000 ) / 1000).toFixed(3); (substitute 1000 and 3 as needed.) Try: (3.1245).toFixed(3) "3.124" (Math.round(3.1245 * 1000) / 1000).toFixed(3) "3.125" x=3.442500000000000001 x.toFixed(3) "3.442" (Math.round(x * 1000) / 1000).toFixed(3) "3.443"
Message was sent while issue was closed.
Committed patchset #11 manually as r202991 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
