Created attachment 305680 [details] patch

Comment on attachment 305680 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=305680&action=review r=me > Source/JavaScriptCore/wasm/WasmMemory.cpp:124 > + if (!fastMemoryEnabled()) nit: if (UNLIKELY(... > Source/JavaScriptCore/wasm/WasmMemory.cpp:128 > + if (!vm.getCTIStub(throwExceptionFromWasmThunkGenerator).size()) nit: if (UNLIKELY(... > Source/JavaScriptCore/wasm/WasmMemory.cpp:129 > + goto fail; "goto"! One of my favorite coding constructs that gets so little use because peeps don't know how to use it right, and fear it. > Source/JavaScriptCore/wasm/WasmMemory.cpp:139 > + if (dequeFastMemory()) nit: if (LIKELY(... > Source/JavaScriptCore/wasm/WasmMemory.cpp:153 > + if (memory) nit: if (LIKELY(memory))

Created attachment 305700 [details] patch > > Source/JavaScriptCore/wasm/WasmMemory.cpp:139 > > + if (dequeFastMemory()) > > nit: if (LIKELY(... > > > Source/JavaScriptCore/wasm/WasmMemory.cpp:153 > > + if (memory) > > nit: if (LIKELY(memory)) I don't know that these two are unlikely: they're not options or odd things. They're certainly undesirable, but I wouldn't call them unlikely. The others you pointed out definitely are, so I marked them appropriately. > > Source/JavaScriptCore/wasm/WasmMemory.cpp:129 > > + goto fail; > > "goto"! One of my favorite coding constructs that gets so little use > because peeps don't know how to use it right, and fear it. Am I using it right? :-p

(In reply to JF Bastien from comment #3) > Created attachment 305700 [details] > patch > > > > Source/JavaScriptCore/wasm/WasmMemory.cpp:139 > > > + if (dequeFastMemory()) > > > > nit: if (LIKELY(... > > > > > Source/JavaScriptCore/wasm/WasmMemory.cpp:153 > > > + if (memory) > > > > nit: if (LIKELY(memory)) > > I don't know that these two are unlikely: they're not options or odd things. > They're certainly undesirable, but I wouldn't call them unlikely. I said "LIKELY", not "UNLIKELY". That's because the alternative is to fail and crash. They better be LIKELY. > The others you pointed out definitely are, so I marked them appropriately. > > > > Source/JavaScriptCore/wasm/WasmMemory.cpp:129 > > > + goto fail; > > > > "goto"! One of my favorite coding constructs that gets so little use > > because peeps don't know how to use it right, and fear it. > > Am I using it right? :-p Totally. =)

> > I don't know that these two are unlikely: they're not options or odd things. > > They're certainly undesirable, but I wouldn't call them unlikely. > > I said "LIKELY", not "UNLIKELY". That's because the alternative is to fail > and crash. They better be LIKELY. It'll only crash if the option is set though. If unset, it'll just return false and generate a slow-mode memory. It's totally normal for this to fail, I just want an option to catch when it does so that I can benchmark knowing we used the fast version! > > The others you pointed out definitely are, so I marked them appropriately. > > > > > > Source/JavaScriptCore/wasm/WasmMemory.cpp:129 > > > > + goto fail; > > > > > > "goto"! One of my favorite coding constructs that gets so little use > > > because peeps don't know how to use it right, and fear it. > > > > Am I using it right? :-p > > Totally. =) 🎉

Comment on attachment 305700 [details] patch Clearing flags on attachment: 305700 Committed r214526: <http://trac.webkit.org/changeset/214526>

All reviewed patches have been landed. Closing bug.

(In reply to JF Bastien from comment #3) > Created attachment 305700 [details] > patch > > > > Source/JavaScriptCore/wasm/WasmMemory.cpp:139 > > > + if (dequeFastMemory()) > > > > nit: if (LIKELY(... > > > > > Source/JavaScriptCore/wasm/WasmMemory.cpp:153 > > > + if (memory) > > > > nit: if (LIKELY(memory)) > > I don't know that these two are unlikely: they're not options or odd things. > They're certainly undesirable, but I wouldn't call them unlikely. > > The others you pointed out definitely are, so I marked them appropriately. > > > > Source/JavaScriptCore/wasm/WasmMemory.cpp:129 > > > + goto fail; > > > > "goto"! One of my favorite coding constructs that gets so little use > > because peeps don't know how to use it right, and fear it. > > Am I using it right? :-p I think more idiomatic JSC style would be to name a lambda fail, and 'return fail()' everywhere you 'goto fail'

Comment on attachment 305680 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=305680&action=review >> Source/JavaScriptCore/wasm/WasmMemory.cpp:129 >> + goto fail; > > "goto"! One of my favorite coding constructs that gets so little use because peeps don't know how to use it right, and fear it. I don't think this is why people don't use "goto"

(In reply to Saam Barati from comment #9) > >> Source/JavaScriptCore/wasm/WasmMemory.cpp:129 > >> + goto fail; > > > > "goto"! One of my favorite coding constructs that gets so little use because peeps don't know how to use it right, and fear it. > > I don't think this is why people don't use "goto" This was a semi-joke, and I was being over the top. My point was that there are valid uses for it, and this was the main one IMHO. Or at least, it was until ... (In reply to Saam Barati from comment #8) > I think more idiomatic JSC style would be to name a lambda fail, and 'return > fail()' everywhere you 'goto fail' I agree with this. This is a much better solution. I knew I should have scratch my head a little longer before saying yes to the gotos.

(In reply to Mark Lam from comment #10) > (In reply to Saam Barati from comment #9) > > >> Source/JavaScriptCore/wasm/WasmMemory.cpp:129 > > >> + goto fail; > > > > > > "goto"! One of my favorite coding constructs that gets so little use because peeps don't know how to use it right, and fear it. > > > > I don't think this is why people don't use "goto" > > This was a semi-joke, and I was being over the top. Yeah I know :) My response is a bit of a tease as well. > are valid uses for it, and this was the main one IMHO. Or at least, it was > until ... > > (In reply to Saam Barati from comment #8) > > I think more idiomatic JSC style would be to name a lambda fail, and 'return > > fail()' everywhere you 'goto fail' > > I agree with this. This is a much better solution. I knew I should have > scratch my head a little longer before saying yes to the gotos. Just to clarify, I'm not trying to argue that one is better than the other. (I could make the argument that LLVM probably produces better code for what we care about in a function like this using the goto variant. It'll either produce smaller code, or code with one fewer function call.) I'm just arguing it's more in line with JSC style not to use goto.

(In reply to Saam Barati from comment #11) > (In reply to Mark Lam from comment #10) > > (In reply to Saam Barati from comment #9) > > > >> Source/JavaScriptCore/wasm/WasmMemory.cpp:129 > > > >> + goto fail; > > > > > > > > "goto"! One of my favorite coding constructs that gets so little use because peeps don't know how to use it right, and fear it. > > > > > > I don't think this is why people don't use "goto" > > > > This was a semi-joke, and I was being over the top. > Yeah I know :) My response is a bit of a tease as well. > > > are valid uses for it, and this was the main one IMHO. Or at least, it was > > until ... > > > > (In reply to Saam Barati from comment #8) > > > I think more idiomatic JSC style would be to name a lambda fail, and 'return > > > fail()' everywhere you 'goto fail' > > > > I agree with this. This is a much better solution. I knew I should have > > scratch my head a little longer before saying yes to the gotos. > Just to clarify, I'm not trying to argue that one is better than the other. > (I could make the argument that LLVM probably produces better code for what > we care about in a function like this using the goto variant. It'll either > produce smaller code, or code with one fewer function call.) I'm just > arguing it's more in line with JSC style not to use goto. Lambda is indeed nicer. Will fix: https://bugs.webkit.org/show_bug.cgi?id=170242 goto is just a reflex for my 2000-era C++, and years of C programming :) I can post-modernize my C++.