Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13520010:
Merge QUIC StrikeRegister code to chromium. (Closed)
|
Created:
7 years, 8 months ago by ramant (doing other things) Modified:
7 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, agl Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMerge QUIC StrikeRegister code to chromium.
R=rch@chromium.org, jar@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192939
Patch Set 1 #Patch Set 2 : adding jar as reviewer #
Total comments: 19
Patch Set 3 : Added DCHECK for byte #
Total comments: 4
Patch Set 4 : Changed DCHECK_EQ to CHECK_EQ #
Messages
Total messages: 21 (0 generated)
Ported StrikeRegister code. PTAL.
A bunch of simplifying comments below for the port. I was curious to ask agl why we're using CHECKs rather than DCHECKS, and had one place where the code might be made a bit easier for me to read (see mention of switch below). https://codereview.chromium.org/13520010/diff/36001/net/quic/crypto/strike_re... File net/quic/crypto/strike_register.cc (right): https://codereview.chromium.org/13520010/diff/36001/net/quic/crypto/strike_re... net/quic/crypto/strike_register.cc:75: CHECK_LT(max_entries, static_cast<unsigned>(1 << 23)); nit: 1u << 23 might work https://codereview.chromium.org/13520010/diff/36001/net/quic/crypto/strike_re... net/quic/crypto/strike_register.cc:262: CHECK_EQ(free_external_nodes.count(i), (size_t)0); nit 0u https://codereview.chromium.org/13520010/diff/36001/net/quic/crypto/strike_re... net/quic/crypto/strike_register.cc:398: case 0xbf: nit: much more readable might be: switch (i->otherbits()) { case 0xff & ~(1 << 7): bit = 0; break case 0xff & ~(1 << 6): bit = 1; break .... https://codereview.chromium.org/13520010/diff/36001/net/quic/crypto/strike_re... net/quic/crypto/strike_register.cc:434: CHECK_EQ(used_external_nodes->count(ext), (size_t)0); agl: Why are these CHECKs, and not DCHECKS? https://codereview.chromium.org/13520010/diff/36001/net/quic/crypto/strike_re... net/quic/crypto/strike_register.cc:439: unsigned byte = i->first / 8; nit: Given the name, perhaps: DCHECK_LE(byte, 0xf); https://codereview.chromium.org/13520010/diff/36001/net/quic/crypto/strike_re... net/quic/crypto/strike_register.cc:442: {0x80, 0x40, 0x20, 0x10, 0x08, 0x04, 0x02, 0x01}; nit: indent 2 more. https://codereview.chromium.org/13520010/diff/36001/net/quic/crypto/strike_re... net/quic/crypto/strike_register.cc:443: CHECK_EQ(bytes[byte] & kMasks[bit] ? true : false, i->second); nit: rather than using ?: use != 0 https://codereview.chromium.org/13520010/diff/36001/net/quic/crypto/strike_re... File net/quic/crypto/strike_register.h (right): https://codereview.chromium.org/13520010/diff/36001/net/quic/crypto/strike_re... net/quic/crypto/strike_register.h:52: kNil = (1 << 31) | 1, nit: to avoid casting to int in cc files, better would be to use const int here, with the definition in the cc file. https://codereview.chromium.org/13520010/diff/36001/net/quic/crypto/strike_re... net/quic/crypto/strike_register.h:131: const unsigned max_entries_; nit: more consistent would be uint32 here.
Hi Jim and agl, Could you please take a look at these changes. thanks. https://chromiumcodereview.appspot.com/13520010/diff/36001/net/quic/crypto/st... File net/quic/crypto/strike_register.cc (right): https://chromiumcodereview.appspot.com/13520010/diff/36001/net/quic/crypto/st... net/quic/crypto/strike_register.cc:75: CHECK_LT(max_entries, static_cast<unsigned>(1 << 23)); On 2013/04/06 02:26:37, jar wrote: > nit: 1u << 23 might work Done. https://chromiumcodereview.appspot.com/13520010/diff/36001/net/quic/crypto/st... net/quic/crypto/strike_register.cc:262: CHECK_EQ(free_external_nodes.count(i), (size_t)0); On 2013/04/06 02:26:37, jar wrote: > nit 0u Done. https://chromiumcodereview.appspot.com/13520010/diff/36001/net/quic/crypto/st... net/quic/crypto/strike_register.cc:398: case 0xbf: On 2013/04/06 02:26:37, jar wrote: > nit: much more readable might be: > > switch (i->otherbits()) { > case 0xff & ~(1 << 7): > bit = 0; > break > case 0xff & ~(1 << 6): > bit = 1; > break > .... Done. https://chromiumcodereview.appspot.com/13520010/diff/36001/net/quic/crypto/st... net/quic/crypto/strike_register.cc:434: CHECK_EQ(used_external_nodes->count(ext), (size_t)0); On 2013/04/06 02:26:37, jar wrote: > agl: Why are these CHECKs, and not DCHECKS? Done. https://chromiumcodereview.appspot.com/13520010/diff/36001/net/quic/crypto/st... net/quic/crypto/strike_register.cc:439: unsigned byte = i->first / 8; On 2013/04/06 02:26:37, jar wrote: > nit: Given the name, perhaps: > > DCHECK_LE(byte, 0xf); Would like to leave this to agl to comment. https://chromiumcodereview.appspot.com/13520010/diff/36001/net/quic/crypto/st... net/quic/crypto/strike_register.cc:442: {0x80, 0x40, 0x20, 0x10, 0x08, 0x04, 0x02, 0x01}; On 2013/04/06 02:26:37, jar wrote: > nit: indent 2 more. Done. https://chromiumcodereview.appspot.com/13520010/diff/36001/net/quic/crypto/st... net/quic/crypto/strike_register.cc:443: CHECK_EQ(bytes[byte] & kMasks[bit] ? true : false, i->second); On 2013/04/06 02:26:37, jar wrote: > nit: rather than using ?: use != 0 Done. https://chromiumcodereview.appspot.com/13520010/diff/36001/net/quic/crypto/st... File net/quic/crypto/strike_register.h (right): https://chromiumcodereview.appspot.com/13520010/diff/36001/net/quic/crypto/st... net/quic/crypto/strike_register.h:52: kNil = (1 << 31) | 1, On 2013/04/06 02:26:37, jar wrote: > nit: to avoid casting to int in cc files, better would be to use const int here, > with the definition in the cc file. Done. https://chromiumcodereview.appspot.com/13520010/diff/36001/net/quic/crypto/st... net/quic/crypto/strike_register.h:131: const unsigned max_entries_; On 2013/04/06 02:26:37, jar wrote: > nit: more consistent would be uint32 here. Done.
Hi Jim, Added code to get 1 in 1000 rand() to get 0.001. Added DCHECK_LE for byte. PTAL. https://chromiumcodereview.appspot.com/13520010/diff/36001/net/quic/crypto/st... File net/quic/crypto/strike_register.cc (right): https://chromiumcodereview.appspot.com/13520010/diff/36001/net/quic/crypto/st... net/quic/crypto/strike_register.cc:439: unsigned byte = i->first / 8; On 2013/04/06 04:20:10, ramant wrote: > On 2013/04/06 02:26:37, jar wrote: > > nit: Given the name, perhaps: > > > > DCHECK_LE(byte, 0xf); > > Would like to leave this to agl to comment. Added DCHECK_LE(byte, 0xff); check
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/13520010/55006
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/13520010/55006
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/13520010/55006
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
NACK. WIll comment on Monday.
https://codereview.chromium.org/13520010/diff/55006/net/quic/crypto/strike_re... File net/quic/crypto/strike_register.cc (right): https://codereview.chromium.org/13520010/diff/55006/net/quic/crypto/strike_re... net/quic/crypto/strike_register.cc:439: DCHECK_EQ(free_external_nodes.count(ext), 0u); ValidateTree is supposed to check the internal consistency of the register. But the DCHECKs mean that it doesn't actually work in Release mode! This code lives here, rather than in a test peer object, because the old code called it every 1000 requests or so as a debugging measure. Perhaps that's no longer needed but it should either be made test only, or should work as is (i.e. with CHECKs).
https://codereview.chromium.org/13520010/diff/55006/net/quic/crypto/strike_re... File net/quic/crypto/strike_register.cc (right): https://codereview.chromium.org/13520010/diff/55006/net/quic/crypto/strike_re... net/quic/crypto/strike_register.cc:439: DCHECK_EQ(free_external_nodes.count(ext), 0u); On 2013/04/08 14:38:52, agl wrote: > ValidateTree is supposed to check the internal consistency of the register. But > the DCHECKs mean that it doesn't actually work in Release mode! This code lives > here, rather than in a test peer object, because the old code called it every > 1000 requests or so as a debugging measure. Perhaps that's no longer needed but > it should either be made test only, or should work as is (i.e. with CHECKs). Out of curiosity, is this code in the internal version, or did it get munged in the merge/port to Chromium?
https://codereview.chromium.org/13520010/diff/55006/net/quic/crypto/strike_re... File net/quic/crypto/strike_register.cc (right): https://codereview.chromium.org/13520010/diff/55006/net/quic/crypto/strike_re... net/quic/crypto/strike_register.cc:439: DCHECK_EQ(free_external_nodes.count(ext), 0u); On 2013/04/08 16:19:03, Ryan Hamilton wrote: > On 2013/04/08 14:38:52, agl wrote: > > ValidateTree is supposed to check the internal consistency of the register. > But > > the DCHECKs mean that it doesn't actually work in Release mode! This code > lives > > here, rather than in a test peer object, because the old code called it every > > 1000 requests or so as a debugging measure. Perhaps that's no longer needed > but > > it should either be made test only, or should work as is (i.e. with CHECKs). > > Out of curiosity, is this code in the internal version, or did it get munged in > the merge/port to Chromium? ValidateTree is in the internal version, yes. It's not called once every 1000 requests etc in QUIC however, it's just called from the tests for now.
Changed DCHECKs to CHECKs. PTAL. https://codereview.chromium.org/13520010/diff/55006/net/quic/crypto/strike_re... File net/quic/crypto/strike_register.cc (right): https://codereview.chromium.org/13520010/diff/55006/net/quic/crypto/strike_re... net/quic/crypto/strike_register.cc:439: DCHECK_EQ(free_external_nodes.count(ext), 0u); On 2013/04/08 14:38:52, agl wrote: > ValidateTree is supposed to check the internal consistency of the register. But > the DCHECKs mean that it doesn't actually work in Release mode! This code lives > here, rather than in a test peer object, because the old code called it every > 1000 requests or so as a debugging measure. Perhaps that's no longer needed but > it should either be made test only, or should work as is (i.e. with CHECKs). Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/13520010/32002
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
Message was sent while issue was closed.
Committed patchset #4 manually as r192939 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
