Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9117037:
Added a Unique ID for a Field Trial containing it's hashed name and the selected group ID. (Closed)
|
Created:
8 years, 11 months ago by MAD Modified:
8 years, 11 months ago CC:
chromium-reviews, brettw-cc_chromium.org, Ilya Sherman Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdded a Unique ID for a Field Trial containing it's hashed name and the selected group ID.
Also returns the list of UIDs of all currently running Field Trials
BUG=None
TEST=FieldTrialTest.UIDs
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=119444
Patch Set 1 #
Total comments: 6
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 6
Patch Set 4 : '' #
Total comments: 18
Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 8
Patch Set 7 : BackToDCHECK #Patch Set 8 : BackToDCHECK2 #
Messages
Total messages: 32 (0 generated)
Is this OK for returning Unique IDentifiers for Field Trials that we could then add to the UMA pings and the crash reports? Thanks! BYE MAD
Oops... Wait... The hashing I used is bad, I need to change it... Sorry... Will send an update after uploading a new version... On 2012/01/24 18:07:18, MAD wrote: > Is this OK for returning Unique IDentifiers for Field Trials that we could then > add to the UMA pings and the crash reports? > > Thanks! > > BYE > MAD
https://chromiumcodereview.appspot.com/9117037/diff/1/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9117037/diff/1/base/metrics/field_tria... base/metrics/field_trial.cc:145: *uid = std::make_pair(BASE_HASH_NAMESPACE::hash_value(name_), group_); IMO, you should consider generating the hash once and for all when the field trial is created, and leaving it in a slot for the field trial. [I was also mistakenly worried about races here... but since you're recording group, and not group name, there is no problem. If you transition to group_name_, and hashing it, it will be critical to put the hash in a slot so that you can atomically access it without a race concern vs group_, which is set asynchronously compared to the group_name_.] https://chromiumcodereview.appspot.com/9117037/diff/1/base/metrics/field_tria... base/metrics/field_trial.cc:287: AutoLock auto_lock(global_->lock_); This is probably ok. I'm always careful to do as little as possible under a lock. Generating the hash does take a bit of time... so the temptation would be to read the list of FieldTrial instances into a vector, release the lock, and then process those instances. Alternatively, if you moved to having a slot for the hashes, then the getter would be more clearly insignificant. https://chromiumcodereview.appspot.com/9117037/diff/1/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9117037/diff/1/base/metrics/field_tria... base/metrics/field_trial.h:97: typedef std::pair<size_t, int> UID; You might want to send two hashes, rather than one. The integer will vary depending on the experiments the programmer includes. It is common to see the names of the experiments changed, such as when folks are trying various window sizes.... decide that some window sizes are very bad... and decide to try some additional sizes. Folks *could* learn to give 0% chance to such obsolete cases, but this is not currently enforceable. Also, size_t should only be used when required, such as for stl:: array indices, or malloc sizes. Better is to use uint32 or such. The typename UID appears to me to be pretty cryptic, and is in fact an abbreviation (which the style guide would not endorse). Given that the type is in the FieldTrial class, we can assume that much is known, and might refine it with something like: NameGroupPair or NameAndChoice other?? Lastly, it is probably better to use a struct than a pair, as readability is improved (and it is not necessary to "remember" which field comes first).
How about this updated version? Thanks! BYE MAD https://chromiumcodereview.appspot.com/9117037/diff/1/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9117037/diff/1/base/metrics/field_tria... base/metrics/field_trial.cc:145: *uid = std::make_pair(BASE_HASH_NAMESPACE::hash_value(name_), group_); On 2012/01/24 19:19:45, jar wrote: > IMO, you should consider generating the hash once and for all when the field > trial is created, and leaving it in a slot for the field trial. > > [I was also mistakenly worried about races here... but since you're recording > group, and not group name, there is no problem. If you transition to > group_name_, and hashing it, it will be critical to put the hash in a slot so > that you can atomically access it without a race concern vs group_, which is set > asynchronously compared to the group_name_.] Yes, good point, I agree... Done... https://chromiumcodereview.appspot.com/9117037/diff/1/base/metrics/field_tria... base/metrics/field_trial.cc:287: AutoLock auto_lock(global_->lock_); On 2012/01/24 19:19:45, jar wrote: > This is probably ok. I'm always careful to do as little as possible under a > lock. Generating the hash does take a bit of time... so the temptation would be > to read the list of FieldTrial instances into a vector, release the lock, and > then process those instances. > > Alternatively, if you moved to having a slot for the hashes, then the getter > would be more clearly insignificant. > Done... https://chromiumcodereview.appspot.com/9117037/diff/1/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9117037/diff/1/base/metrics/field_tria... base/metrics/field_trial.h:97: typedef std::pair<size_t, int> UID; On 2012/01/24 19:19:45, jar wrote: > You might want to send two hashes, rather than one. The integer will vary > depending on the experiments the programmer includes. It is common to see the > names of the experiments changed, such as when folks are trying various window > sizes.... decide that some window sizes are very bad... and decide to try some > additional sizes. Folks *could* learn to give 0% chance to such obsolete cases, > but this is not currently enforceable. > > Also, size_t should only be used when required, such as for stl:: array indices, > or malloc sizes. Better is to use uint32 or such. > > The typename UID appears to me to be pretty cryptic, and is in fact an > abbreviation (which the style guide would not endorse). Given that the type is > in the FieldTrial class, we can assume that much is known, and might refine it > with something like: > NameGroupPair > or > NameAndChoice > other?? > > Lastly, it is probably better to use a struct than a pair, as readability is > improved (and it is not necessary to "remember" which field comes first). OK! Done... Thanks! :-)
https://chromiumcodereview.appspot.com/9117037/diff/2004/base/metrics/field_t... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9117037/diff/2004/base/metrics/field_t... base/metrics/field_trial.cc:52: enable_field_trial_(true) { please provide initialization for the group_name_hash_ https://chromiumcodereview.appspot.com/9117037/diff/2004/base/metrics/field_t... base/metrics/field_trial.cc:146: if (group_ == kNotFinalized) You need to check for the (reserved) initial value of group_name_hash_. https://chromiumcodereview.appspot.com/9117037/diff/2004/base/metrics/field_t... File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9117037/diff/2004/base/metrics/field_t... base/metrics/field_trial.h:239: // Is not valid while group_ is equal to kNotFinalized. Since you don't have synchronization, you need to have a reserved value that is uninitialized. You can't both *test* for the group_ value, and read this group_name_hash_. At best, you can facilitate the race in a way that is decodable, if you initialize the group_name_hash_ to a reserved value... but once you do that, you may as well look for that value, rather than look at teh separate slot group_.
Yeah, I was thinking about that but didn't know if I could safely rely on the hash never being 0... But I guess we can catch it early enough and simply warn the developer to change the name... Thanks! Is this OK? BYE MAD http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.cc#... base/metrics/field_trial.cc:52: enable_field_trial_(true) { On 2012/01/24 21:51:23, jar wrote: > please provide initialization for the group_name_hash_ Done. http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.cc#... base/metrics/field_trial.cc:146: if (group_ == kNotFinalized) On 2012/01/24 21:51:23, jar wrote: > You need to check for the (reserved) initial value of group_name_hash_. Done. http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.h File base/metrics/field_trial.h (right): http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.h#n... base/metrics/field_trial.h:239: // Is not valid while group_ is equal to kNotFinalized. On 2012/01/24 21:51:23, jar wrote: > Since you don't have synchronization, you need to have a reserved value that is > uninitialized. You can't both *test* for the group_ value, and read this > group_name_hash_. > > At best, you can facilitate the race in a way that is decodable, if you > initialize the group_name_hash_ to a reserved value... but once you do that, you > may as well look for that value, rather than look at teh separate slot group_. Done.
You probably have two (easy) choices: a) Hope that a hash won't collide with a reserved value (with uint32, and a good hash, you have a pretty low chance of hitting any reserved value). That looks like what you implemented. I guess you could add an assertion that prevents folks from defining a trial version with a hash that colides with the reserved value. b) Change your hash function so it never returns the "reserved value ". This means that when it *does* try to return a restricted value (from SHA or MD5 or whatever), you just have to return "something else." Most trivially, you could (for example) reserve 0, and then when the regular hash seems like it was going to return 0, return either a constant, or some other minimally (non-zero) hash, such as the first couple of (non-null) characters of the string. Both will (mostly) work... but (b) will guarantee correctness, and be a little less of a burden on users. By sticking with uint32, you get atomic loading on all our platforms... and don't need to worry about locks. Jim On Tue, Jan 24, 2012 at 4:14 PM, <mad@chromium.org> wrote: > Yeah, I was thinking about that but didn't know if I could safely rely on > the > hash never being 0... But I guess we can catch it early enough and simply > warn > the developer to change the name... > > Thanks! > > Is this OK? > > BYE > MAD > > > > http://codereview.chromium.**org/9117037/diff/2004/base/** > metrics/field_trial.cc<http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.cc> > File base/metrics/field_trial.cc (right): > > http://codereview.chromium.**org/9117037/diff/2004/base/** > metrics/field_trial.cc#**newcode52<http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.cc#newcode52> > base/metrics/field_trial.cc:**52: enable_field_trial_(true) { > > On 2012/01/24 21:51:23, jar wrote: > >> please provide initialization for the group_name_hash_ >> > > Done. > > http://codereview.chromium.**org/9117037/diff/2004/base/** > metrics/field_trial.cc#**newcode146<http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.cc#newcode146> > > base/metrics/field_trial.cc:**146: if (group_ == kNotFinalized) > On 2012/01/24 21:51:23, jar wrote: > >> You need to check for the (reserved) initial value of >> > group_name_hash_. > > Done. > > http://codereview.chromium.**org/9117037/diff/2004/base/** > metrics/field_trial.h<http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.h> > File base/metrics/field_trial.h (right): > > http://codereview.chromium.**org/9117037/diff/2004/base/** > metrics/field_trial.h#**newcode239<http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.h#newcode239> > > base/metrics/field_trial.h:**239: // Is not valid while group_ is equal to > kNotFinalized. > On 2012/01/24 21:51:23, jar wrote: > >> Since you don't have synchronization, you need to have a reserved >> > value that is > >> uninitialized. You can't both *test* for the group_ value, and read >> > this > >> group_name_hash_. >> > > At best, you can facilitate the race in a way that is decodable, if >> > you > >> initialize the group_name_hash_ to a reserved value... but once you do >> > that, you > >> may as well look for that value, rather than look at teh separate slot >> > group_. > > Done. > > http://codereview.chromium.**org/9117037/<http://codereview.chromium.org/9117... >
...or an even nicer variant of (b): If the low order 32 bits match the reserved value, then consider returning the high order 32 bits instead. If they too match the reserved hash, then return (for example) a constant (other than the reserved hash). That will give you a very uniform distribution across the space of non-reserved ints, with only a 1 in 2^64 chance of having to fall back to the trvial hash (hence statistically insignifciant for our purposes). YMMV... but it will be fast, and always correct. Jim On Tue, Jan 24, 2012 at 7:37 PM, Jim Roskind <jar@chromium.org> wrote: > You probably have two (easy) choices: > > a) Hope that a hash won't collide with a reserved value (with uint32, and > a good hash, you have a pretty low chance of hitting any reserved value). > That looks like what you implemented. I guess you could add an assertion > that prevents folks from defining a trial version with a hash that colides > with the reserved value. > > b) Change your hash function so it never returns the "reserved value ". > This means that when it *does* try to return a restricted value (from SHA > or MD5 or whatever), you just have to return "something else." Most > trivially, you could (for example) reserve 0, and then when the regular > hash seems like it was going to return 0, return either a constant, or some > other minimally (non-zero) hash, such as the first couple of (non-null) > characters of the string. > > Both will (mostly) work... but (b) will guarantee correctness, and be a > little less of a burden on users. By sticking with uint32, you get atomic > loading on all our platforms... and don't need to worry about locks. > > Jim > > > On Tue, Jan 24, 2012 at 4:14 PM, <mad@chromium.org> wrote: > >> Yeah, I was thinking about that but didn't know if I could safely rely on >> the >> hash never being 0... But I guess we can catch it early enough and simply >> warn >> the developer to change the name... >> >> Thanks! >> >> Is this OK? >> >> BYE >> MAD >> >> >> >> http://codereview.chromium.**org/9117037/diff/2004/base/** >> metrics/field_trial.cc<http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.cc> >> File base/metrics/field_trial.cc (right): >> >> http://codereview.chromium.**org/9117037/diff/2004/base/** >> metrics/field_trial.cc#**newcode52<http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.cc#newcode52> >> base/metrics/field_trial.cc:**52: enable_field_trial_(true) { >> >> On 2012/01/24 21:51:23, jar wrote: >> >>> please provide initialization for the group_name_hash_ >>> >> >> Done. >> >> http://codereview.chromium.**org/9117037/diff/2004/base/** >> metrics/field_trial.cc#**newcode146<http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.cc#newcode146> >> >> base/metrics/field_trial.cc:**146: if (group_ == kNotFinalized) >> On 2012/01/24 21:51:23, jar wrote: >> >>> You need to check for the (reserved) initial value of >>> >> group_name_hash_. >> >> Done. >> >> http://codereview.chromium.**org/9117037/diff/2004/base/** >> metrics/field_trial.h<http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.h> >> File base/metrics/field_trial.h (right): >> >> http://codereview.chromium.**org/9117037/diff/2004/base/** >> metrics/field_trial.h#**newcode239<http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.h#newcode239> >> >> base/metrics/field_trial.h:**239: // Is not valid while group_ is equal >> to >> kNotFinalized. >> On 2012/01/24 21:51:23, jar wrote: >> >>> Since you don't have synchronization, you need to have a reserved >>> >> value that is >> >>> uninitialized. You can't both *test* for the group_ value, and read >>> >> this >> >>> group_name_hash_. >>> >> >> At best, you can facilitate the race in a way that is decodable, if >>> >> you >> >>> initialize the group_name_hash_ to a reserved value... but once you do >>> >> that, you >> >>> may as well look for that value, rather than look at teh separate slot >>> >> group_. >> >> Done. >> >> http://codereview.chromium.**org/9117037/<http://codereview.chromium.org/9117... >> > >
Yeah, I implemented A) with a DCHECK saying: "Bad luck, that name can't be used" I'll add your good suggestion of using the next set of 32 bits returned by SHA1HashBytes when the reserved value (currently 0) is found in the first set, and if this is the reserved value again, simply return a fallback constant (e.g., 1)... Would that be OK for you? On Wed, Jan 25, 2012 at 7:56 AM, Jim Roskind <jar@chromium.org> wrote: > ...or an even nicer variant of (b): > > If the low order 32 bits match the reserved value, then consider returning > the high order 32 bits instead. If they too match the reserved hash, then > return (for example) a constant (other than the reserved hash). That will > give you a very uniform distribution across the space of non-reserved ints, > with only a 1 in 2^64 chance of having to fall back to the trvial hash > (hence statistically insignifciant for our purposes). > > YMMV... but it will be fast, and always correct. > > Jim > > On Tue, Jan 24, 2012 at 7:37 PM, Jim Roskind <jar@chromium.org> wrote: > >> You probably have two (easy) choices: >> >> a) Hope that a hash won't collide with a reserved value (with uint32, and >> a good hash, you have a pretty low chance of hitting any reserved value). >> That looks like what you implemented. I guess you could add an assertion >> that prevents folks from defining a trial version with a hash that colides >> with the reserved value. >> >> b) Change your hash function so it never returns the "reserved value ". >> This means that when it *does* try to return a restricted value (from SHA >> or MD5 or whatever), you just have to return "something else." Most >> trivially, you could (for example) reserve 0, and then when the regular >> hash seems like it was going to return 0, return either a constant, or some >> other minimally (non-zero) hash, such as the first couple of (non-null) >> characters of the string. >> >> Both will (mostly) work... but (b) will guarantee correctness, and be a >> little less of a burden on users. By sticking with uint32, you get atomic >> loading on all our platforms... and don't need to worry about locks. >> >> Jim >> >> >> On Tue, Jan 24, 2012 at 4:14 PM, <mad@chromium.org> wrote: >> >>> Yeah, I was thinking about that but didn't know if I could safely rely >>> on the >>> hash never being 0... But I guess we can catch it early enough and >>> simply warn >>> the developer to change the name... >>> >>> Thanks! >>> >>> Is this OK? >>> >>> BYE >>> MAD >>> >>> >>> >>> http://codereview.chromium.**org/9117037/diff/2004/base/** >>> metrics/field_trial.cc<http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.cc> >>> File base/metrics/field_trial.cc (right): >>> >>> http://codereview.chromium.**org/9117037/diff/2004/base/** >>> metrics/field_trial.cc#**newcode52<http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.cc#newcode52> >>> base/metrics/field_trial.cc:**52: enable_field_trial_(true) { >>> >>> On 2012/01/24 21:51:23, jar wrote: >>> >>>> please provide initialization for the group_name_hash_ >>>> >>> >>> Done. >>> >>> http://codereview.chromium.**org/9117037/diff/2004/base/** >>> metrics/field_trial.cc#**newcode146<http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.cc#newcode146> >>> >>> base/metrics/field_trial.cc:**146: if (group_ == kNotFinalized) >>> On 2012/01/24 21:51:23, jar wrote: >>> >>>> You need to check for the (reserved) initial value of >>>> >>> group_name_hash_. >>> >>> Done. >>> >>> http://codereview.chromium.**org/9117037/diff/2004/base/** >>> metrics/field_trial.h<http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.h> >>> File base/metrics/field_trial.h (right): >>> >>> http://codereview.chromium.**org/9117037/diff/2004/base/** >>> metrics/field_trial.h#**newcode239<http://codereview.chromium.org/9117037/diff/2004/base/metrics/field_trial.h#newcode239> >>> >>> base/metrics/field_trial.h:**239: // Is not valid while group_ is equal >>> to >>> kNotFinalized. >>> On 2012/01/24 21:51:23, jar wrote: >>> >>>> Since you don't have synchronization, you need to have a reserved >>>> >>> value that is >>> >>>> uninitialized. You can't both *test* for the group_ value, and read >>>> >>> this >>> >>>> group_name_hash_. >>>> >>> >>> At best, you can facilitate the race in a way that is decodable, if >>>> >>> you >>> >>>> initialize the group_name_hash_ to a reserved value... but once you do >>>> >>> that, you >>> >>>> may as well look for that value, rather than look at teh separate slot >>>> >>> group_. >>> >>> Done. >>> >>> http://codereview.chromium.**org/9117037/<http://codereview.chromium.org/9117... >>> >> >> >
https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.cc:197: SHA1HashBytes(reinterpret_cast<const unsigned char*>(name.c_str()), I think you can use |SHA1HashString()| instead of |SHA1HashBytes()| here. https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.cc:308: if (!global_) Should you clear |name_group_ids| in this case (and also before appending)? https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.h:97: struct NameGroupId{ Nit: Add a space before {. https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.h:161: // elected. Returns true if a winner is returned, false otherwise. Can you reword this to either explain what you mean by "winner", or using different terms? Also "Returns true if a winner is returned" could be worded better, even if you do wish to keep the term "winner". https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.h:207: // Creates unique identifier for the trial by hashing the name string. The comment should probably mention that this hash function is used for hashing both name and group name, not just the name. https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.h:317: // Returns an array of Unique IDs for each currently running Field Trials. Can you expand this comment to explain what "currently running" means? e.g. that if a group hasn't been chosen yet, then it is not included.
Thanks Alexei, I fixed all but two of you comments, where I ask you a question instead... I'm also waiting for an answer from Jim whether we should change the DCHECK for a collision with the reserved value or not... Will address these open questions in a subsequent upload... The last upload addresses all you comments about nits/comments in the header file... BYE MAD https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.cc:197: SHA1HashBytes(reinterpret_cast<const unsigned char*>(name.c_str()), On 2012/01/25 15:25:19, Alexei Svitkine wrote: > I think you can use |SHA1HashString()| instead of |SHA1HashBytes()| here. I copied the behavior from HashClientId... Should we change both? https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.cc:308: if (!global_) On 2012/01/25 15:25:19, Alexei Svitkine wrote: > Should you clear |name_group_ids| in this case (and also before appending)? Again, I copied the behavior from StatesToString which DCHECK that the output argument is empty... Should we change both? https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.h:97: struct NameGroupId{ On 2012/01/25 15:25:19, Alexei Svitkine wrote: > Nit: Add a space before {. Done. https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.h:161: // elected. Returns true if a winner is returned, false otherwise. On 2012/01/25 15:25:19, Alexei Svitkine wrote: > Can you reword this to either explain what you mean by "winner", or using > different terms? Also "Returns true if a winner is returned" could be worded > better, even if you do wish to keep the term "winner". Done. https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.h:207: // Creates unique identifier for the trial by hashing the name string. On 2012/01/25 15:25:19, Alexei Svitkine wrote: > The comment should probably mention that this hash function is used for hashing > both name and group name, not just the name. Done. https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.h:317: // Returns an array of Unique IDs for each currently running Field Trials. On 2012/01/25 15:25:19, Alexei Svitkine wrote: > Can you expand this comment to explain what "currently running" means? e.g. that > if a group hasn't been chosen yet, then it is not included. Done.
On Wed, Jan 25, 2012 at 9:02 AM, <mad@chromium.org> wrote: > Thanks Alexei, I fixed all but two of you comments, where I ask you a > question > instead... > > I'm also waiting for an answer from Jim whether we should change the > DCHECK for > a collision with the reserved value or not... > I think the reserved value, that is automatically avoided, is a better solution. Less to explain to users. Thanks, Jim > > Will address these open questions in a subsequent upload... The last upload > addresses all you comments about nits/comments in the header file... > > BYE > MAD > > > > > https://chromiumcodereview.**appspot.com/9117037/diff/8001/** > base/metrics/field_trial.cc<https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_trial.cc> > File base/metrics/field_trial.cc (right): > > https://chromiumcodereview.**appspot.com/9117037/diff/8001/** > base/metrics/field_trial.cc#**newcode197<https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_trial.cc#newcode197> > base/metrics/field_trial.cc:**197: SHA1HashBytes(reinterpret_**cast<const > unsigned char*>(name.c_str()), > On 2012/01/25 15:25:19, Alexei Svitkine wrote: > >> I think you can use |SHA1HashString()| instead of |SHA1HashBytes()| >> > here. > > I copied the behavior from HashClientId... Should we change both? > > > https://chromiumcodereview.**appspot.com/9117037/diff/8001/** > base/metrics/field_trial.cc#**newcode308<https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_trial.cc#newcode308> > base/metrics/field_trial.cc:**308: if (!global_) > On 2012/01/25 15:25:19, Alexei Svitkine wrote: > >> Should you clear |name_group_ids| in this case (and also before >> > appending)? > > Again, I copied the behavior from StatesToString which DCHECK that the > output argument is empty... Should we change both? > > > https://chromiumcodereview.**appspot.com/9117037/diff/8001/** > base/metrics/field_trial.h<https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_trial.h> > File base/metrics/field_trial.h (right): > > https://chromiumcodereview.**appspot.com/9117037/diff/8001/** > base/metrics/field_trial.h#**newcode97<https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_trial.h#newcode97> > base/metrics/field_trial.h:97: struct NameGroupId{ > On 2012/01/25 15:25:19, Alexei Svitkine wrote: > >> Nit: Add a space before {. >> > > Done. > > > https://chromiumcodereview.**appspot.com/9117037/diff/8001/** > base/metrics/field_trial.h#**newcode161<https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_trial.h#newcode161> > base/metrics/field_trial.h:**161: // elected. Returns true if a winner is > returned, false otherwise. > On 2012/01/25 15:25:19, Alexei Svitkine wrote: > >> Can you reword this to either explain what you mean by "winner", or >> > using > >> different terms? Also "Returns true if a winner is returned" could be >> > worded > >> better, even if you do wish to keep the term "winner". >> > > Done. > > > https://chromiumcodereview.**appspot.com/9117037/diff/8001/** > base/metrics/field_trial.h#**newcode207<https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_trial.h#newcode207> > base/metrics/field_trial.h:**207: // Creates unique identifier for the > trial by hashing the name string. > On 2012/01/25 15:25:19, Alexei Svitkine wrote: > >> The comment should probably mention that this hash function is used >> > for hashing > >> both name and group name, not just the name. >> > > Done. > > > https://chromiumcodereview.**appspot.com/9117037/diff/8001/** > base/metrics/field_trial.h#**newcode317<https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_trial.h#newcode317> > base/metrics/field_trial.h:**317: // Returns an array of Unique IDs for > each currently running Field Trials. > On 2012/01/25 15:25:19, Alexei Svitkine wrote: > >> Can you expand this comment to explain what "currently running" means? >> > e.g. that > >> if a group hasn't been chosen yet, then it is not included. >> > > Done. > > https://chromiumcodereview.**appspot.com/9117037/<https://chromiumcodereview.... >
https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.cc:195: // even for nearly-identical name. "nearly-identical name" -> "nearly-identical inputs" (more grammatical). https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.cc:197: SHA1HashBytes(reinterpret_cast<const unsigned char*>(name.c_str()), On 2012/01/25 17:02:33, MAD wrote: > On 2012/01/25 15:25:19, Alexei Svitkine wrote: > > I think you can use |SHA1HashString()| instead of |SHA1HashBytes()| here. > > I copied the behavior from HashClientId... Should we change both? Looking closer at |SHA1HashString()|, it returns its output as a string - which is not a problem since we can still cast its data() to bits for the same effect, but is less efficient because it involves an extra temporary string and probably an allocation. So actually, I'm okay with keeping it as it is, since it is slightly more efficient. https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.cc:308: if (!global_) On 2012/01/25 17:02:33, MAD wrote: > On 2012/01/25 15:25:19, Alexei Svitkine wrote: > > Should you clear |name_group_ids| in this case (and also before appending)? > > Again, I copied the behavior from StatesToString which DCHECK that the output > argument is empty... Should we change both? I'm okay with the DCHECK(), though then it should be before if (!global_) and the header comment should mention that |name_group_ids| must be empty initially. Can you make those changes both here and StatesToString()?
OK, thanks again... Uploaded a new patch that should take care of all current comments... Any more comments? BYE MAD https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.cc:195: // even for nearly-identical name. On 2012/01/25 17:40:16, Alexei Svitkine wrote: > "nearly-identical name" -> "nearly-identical inputs" (more grammatical). Done. https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.cc:197: SHA1HashBytes(reinterpret_cast<const unsigned char*>(name.c_str()), On 2012/01/25 17:40:16, Alexei Svitkine wrote: > On 2012/01/25 17:02:33, MAD wrote: > > On 2012/01/25 15:25:19, Alexei Svitkine wrote: > > > I think you can use |SHA1HashString()| instead of |SHA1HashBytes()| here. > > > > I copied the behavior from HashClientId... Should we change both? > > Looking closer at |SHA1HashString()|, it returns its output as a string - which > is not a problem since we can still cast its data() to bits for the same effect, > but is less efficient because it involves an extra temporary string and probably > an allocation. > > So actually, I'm okay with keeping it as it is, since it is slightly more > efficient. OK, Thanks for checking... ;-) https://chromiumcodereview.appspot.com/9117037/diff/8001/base/metrics/field_t... base/metrics/field_trial.cc:308: if (!global_) On 2012/01/25 17:40:16, Alexei Svitkine wrote: > On 2012/01/25 17:02:33, MAD wrote: > > On 2012/01/25 15:25:19, Alexei Svitkine wrote: > > > Should you clear |name_group_ids| in this case (and also before appending)? > > > > Again, I copied the behavior from StatesToString which DCHECK that the output > > argument is empty... Should we change both? > > I'm okay with the DCHECK(), though then it should be before if (!global_) and > the header comment should mention that |name_group_ids| must be empty initially. > > Can you make those changes both here and StatesToString()? Done.
https://chromiumcodereview.appspot.com/9117037/diff/14007/base/metrics/field_... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9117037/diff/14007/base/metrics/field_... base/metrics/field_trial.cc:196: static const size_t kStep = sizeof(uint32) / sizeof(char); You can make this logic a little simpler - if you initialize |bits| to the first uint32, then you can just increment it via |bits++| while checking bits < &sha1_hash[kSHA1Length]. https://chromiumcodereview.appspot.com/9117037/diff/14007/base/metrics/field_... base/metrics/field_trial.cc:205: *bits = kFallbackHashValue; While this case seems extremely unlikely, its also pretty scary. Would it be possible to have a test that will check any FieldTrials in the code that none of their names and possible groups hit this? Then, this code could have a CHECK() for this.
https://chromiumcodereview.appspot.com/9117037/diff/14007/base/metrics/field_... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9117037/diff/14007/base/metrics/field_... base/metrics/field_trial.cc:196: static const size_t kStep = sizeof(uint32) / sizeof(char); +1 for some variant of Alexei's suggestion. nit: If you did stay with the approach on line 196, the style guide is clear that much better would be: kStep = sizeof(bits) / sizeof(sha1_hash[0]); Always try to take the sizeof variables, rather than their types. On 2012/01/25 18:59:39, Alexei Svitkine wrote: > You can make this logic a little simpler - if you initialize |bits| to the first > uint32, then you can just increment it via |bits++| while checking bits < > &sha1_hash[kSHA1Length]. https://chromiumcodereview.appspot.com/9117037/diff/14007/base/metrics/field_... base/metrics/field_trial.cc:199: bits = reinterpret_cast<uint32*>(&sha1_hash[offset]); I suspect this code is not safe for big-endian vs little-endian machines. The problem is only significant because we'll send this hash to an external server, that has expectations about the relationship between the strings and its hash. I asked around, and was told that we don't support big-endian. To be sure this works, please include a test case that has the expected hash values for some known string. This way, if someone tried to port it, the test would blow up, and identify the issue. https://chromiumcodereview.appspot.com/9117037/diff/14007/base/metrics/field_... base/metrics/field_trial.cc:205: *bits = kFallbackHashValue; At best, a DCHECK is called for IMO... but the point of using this secondary hash was to avoid the need for the DCHECK/CHECK You should have a unit test that asserted that we don't hit this fallback in our tests (as that is probably a hint that the logic is broken). On 2012/01/25 18:59:39, Alexei Svitkine wrote: > While this case seems extremely unlikely, its also pretty scary. Would it be > possible to have a test that will check any FieldTrials in the code that none of > their names and possible groups hit this? > > Then, this code could have a CHECK() for this.
drive-by... https://chromiumcodereview.appspot.com/9117037/diff/14007/base/metrics/field_... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9117037/diff/14007/base/metrics/field_... base/metrics/field_trial.cc:196: static const size_t kStep = sizeof(uint32) / sizeof(char); A related note is that you're currently stepping through 32-bits at a time to avoid a reserved value - you could just as easily step through 8 bits at a time, thus giving you many more possible values from the 160-bit hash before you exhaust it. Alexei's proposal would also step 4 bytes at a time. Although it would lead to an unaligned memory access, given that you're doing a SHA-1 hash I really don't think that would be significant. E.g. I don't understand why you need kStep to be 4 instead of 1. I personally think this would be much more readable as: uint32* bits = reinterpret_cast<uint32*>(sha1_hash); size_t offset = 0; while (*bits == kReservedHashValue && ++offset <= (kSha1Length - sizeof(uint32)) { bits = reinterpret_cast<uint32*>(sha1_hash + offset); } DCHECK(*bits == kReservedHashValue) << "PICK A BETTER NAME FOO!"; On 2012/01/25 18:59:39, Alexei Svitkine wrote: > You can make this logic a little simpler - if you initialize |bits| to the first > uint32, then you can just increment it via |bits++| while checking bits < > &sha1_hash[kSHA1Length].
drive by response to the drive by: :-) You get a much more even distribution if you step 4 bytes at a time. The fact that you have to step at all *only* happens when the first 4 bytes are 0, suggests that stepping one at a time will only use a tiny portion of the hash range (when you do have to step). ...and then there is a misaligned access to deal with... but that is indeed less problematic. Jim On Wed, Jan 25, 2012 at 12:03 PM, <ian@chromium.org> wrote: > drive-by... > > > > https://chromiumcodereview.**appspot.com/9117037/diff/** > 14007/base/metrics/field_**trial.cc<https://chromiumcodereview.appspot.com/9117037/diff/14007/base/metrics/field_trial.cc> > File base/metrics/field_trial.cc (right): > > https://chromiumcodereview.**appspot.com/9117037/diff/** > 14007/base/metrics/field_**trial.cc#newcode196<https://chromiumcodereview.appspot.com/9117037/diff/14007/base/metrics/field_trial.cc#newcode196> > base/metrics/field_trial.cc:**196: static const size_t kStep = > sizeof(uint32) / sizeof(char); > A related note is that you're currently stepping through 32-bits at a > time to avoid a reserved value - you could just as easily step through 8 > bits at a time, thus giving you many more possible values from the > 160-bit hash before you exhaust it. Alexei's proposal would also step 4 > bytes at a time. Although it would lead to an unaligned memory access, > given that you're doing a SHA-1 hash I really don't think that would be > significant. > > E.g. I don't understand why you need kStep to be 4 instead of 1. I > personally think this would be much more readable as: > > > uint32* bits = reinterpret_cast<uint32*>(**sha1_hash); > size_t offset = 0; > while (*bits == kReservedHashValue && ++offset <= (kSha1Length - > sizeof(uint32)) { > bits = reinterpret_cast<uint32*>(**sha1_hash + offset); > } > DCHECK(*bits == kReservedHashValue) << "PICK A BETTER NAME FOO!"; > > > On 2012/01/25 18:59:39, Alexei Svitkine wrote: > >> You can make this logic a little simpler - if you initialize |bits| to >> > the first > >> uint32, then you can just increment it via |bits++| while checking >> > bits < > >> &sha1_hash[kSHA1Length]. >> > > https://chromiumcodereview.**appspot.com/9117037/<https://chromiumcodereview.... >
The probability of ever hitting any of this code is so close to zero it's not even funny. It would be equally easy, IMO, to just not do any stepping at all and just have a DCHECK in there DCHECK(*bits) << "Choose a different name for field trial " << field_trial_name; and remove that code entirely. On 2012/01/25 20:09:50, jar wrote: > drive by response to the drive by: :-) > > You get a much more even distribution if you step 4 bytes at a time. The > fact that you have to step at all *only* happens when the first 4 bytes are > 0, suggests that stepping one at a time will only use a tiny portion of the > hash range (when you do have to step). > > ...and then there is a misaligned access to deal with... but that is indeed > less problematic. > > Jim > > > On Wed, Jan 25, 2012 at 12:03 PM, <mailto:ian@chromium.org> wrote: > > > drive-by... > > > > > > > > https://chromiumcodereview.**appspot.com/9117037/diff/** > > > 14007/base/metrics/field_**trial.cc<https://chromiumcodereview.appspot.com/9117037/diff/14007/base/metrics/field_trial.cc> > > File base/metrics/field_trial.cc (right): > > > > https://chromiumcodereview.**appspot.com/9117037/diff/** > > > 14007/base/metrics/field_**trial.cc#newcode196<https://chromiumcodereview.appspot.com/9117037/diff/14007/base/metrics/field_trial.cc#newcode196> > > base/metrics/field_trial.cc:**196: static const size_t kStep = > > sizeof(uint32) / sizeof(char); > > A related note is that you're currently stepping through 32-bits at a > > time to avoid a reserved value - you could just as easily step through 8 > > bits at a time, thus giving you many more possible values from the > > 160-bit hash before you exhaust it. Alexei's proposal would also step 4 > > bytes at a time. Although it would lead to an unaligned memory access, > > given that you're doing a SHA-1 hash I really don't think that would be > > significant. > > > > E.g. I don't understand why you need kStep to be 4 instead of 1. I > > personally think this would be much more readable as: > > > > > > uint32* bits = reinterpret_cast<uint32*>(**sha1_hash); > > size_t offset = 0; > > while (*bits == kReservedHashValue && ++offset <= (kSha1Length - > > sizeof(uint32)) { > > bits = reinterpret_cast<uint32*>(**sha1_hash + offset); > > } > > DCHECK(*bits == kReservedHashValue) << "PICK A BETTER NAME FOO!"; > > > > > > On 2012/01/25 18:59:39, Alexei Svitkine wrote: > > > >> You can make this logic a little simpler - if you initialize |bits| to > >> > > the first > > > >> uint32, then you can just increment it via |bits++| while checking > >> > > bits < > > > >> &sha1_hash[kSHA1Length]. > >> > > > > > https://chromiumcodereview.**appspot.com/9117037/%3Chttps://chromiumcoderevie...> > >
On Wed, Jan 25, 2012 at 4:03 PM, <ian@chromium.org> wrote: > The probability of ever hitting any of this code is so close to zero it's > not > even funny. It would be equally easy, IMO, to just not do any stepping at > all > and just have a DCHECK in there DCHECK(*bits) << "Choose a different name > for > field trial " << field_trial_name; > > and remove that code entirely. I agree - especially if we can have a test that iterates all registered field trials and calls this for every possible name / group_name. -Alexei
That (with a DCHECK or CHECK) was almost the previous CL upload. IMO, it is easier to just be dead right, and not have to explain to anyone (ever) that they need to change a name for a silly reason. It is (as you point out, for lack of an full iteration test), painful to test... and that complexity far exceeds the complexity of the tiny if/else/if code. Jim On Wed, Jan 25, 2012 at 1:58 PM, Alexei Svitkine <asvitkine@chromium.org>wrote: > > > On Wed, Jan 25, 2012 at 4:03 PM, <ian@chromium.org> wrote: > >> The probability of ever hitting any of this code is so close to zero it's >> not >> even funny. It would be equally easy, IMO, to just not do any stepping at >> all >> and just have a DCHECK in there DCHECK(*bits) << "Choose a different name >> for >> field trial " << field_trial_name; >> >> and remove that code entirely. > > > I agree - especially if we can have a test that iterates all registered > field trials and calls this for every possible name / group_name. > > -Alexei > >
The probability of any given string a developer ads triggering this is somewhere around 1 in 4 billion. So, you're talking about an infinitesimally small probability that a given name will hit this condition, combined with the probability that a developer never actually exercises his code in a debug build... again, it just doesn't seem like worth worrying about to me. Having an if (hash == reserved) hash == some_constant I agree isn't a huge overhead and is fine, but doing anything more complex (such as trying multiple iterations of hashing / stepping through the hash) seems way over-engineered and likely to induce error somewhere (I guarantee you that someone will forget that logic somewhere in server-side analysis code.) On 2012/01/25 22:04:45, jar wrote: > That (with a DCHECK or CHECK) was almost the previous CL upload. > > IMO, it is easier to just be dead right, and not have to explain to anyone > (ever) that they need to change a name for a silly reason. It is (as you > point out, for lack of an full iteration test), painful to test... and that > complexity far exceeds the complexity of the tiny if/else/if code. > > Jim > > On Wed, Jan 25, 2012 at 1:58 PM, Alexei Svitkine <asvitkine@chromium.org>wrote: > > > > > > > On Wed, Jan 25, 2012 at 4:03 PM, <mailto:ian@chromium.org> wrote: > > > >> The probability of ever hitting any of this code is so close to zero it's > >> not > >> even funny. It would be equally easy, IMO, to just not do any stepping at > >> all > >> and just have a DCHECK in there DCHECK(*bits) << "Choose a different name > >> for > >> field trial " << field_trial_name; > >> > >> and remove that code entirely. > > > > > > I agree - especially if we can have a test that iterates all registered > > field trials and calls this for every possible name / group_name. > > > > -Alexei > > > >
I agree with Ian, I forgot the fact that we must do the same on the server side... So we'll be able to let the developer know instantly as they add their experiment information on the server side. So this will not be so hard to understand for a developer, so I'll bring back the initial version which was simply DCHECK'ing, and I could add the assignment to the fallback value, just for completion sake, but we should never get there since developers won't be able to use the names that would generate a 0 hash, we'll warn them as they register the experiment on the server side... CL on the way... On Wed, Jan 25, 2012 at 8:22 PM, <ian@chromium.org> wrote: > The probability of any given string a developer ads triggering this is > somewhere > around 1 in 4 billion. So, you're talking about an infinitesimally small > probability that a given name will hit this condition, combined with the > probability that a developer never actually exercises his code in a debug > build... again, it just doesn't seem like worth worrying about to me. > > Having an if (hash == reserved) hash == some_constant I agree isn't a huge > overhead and is fine, but doing anything more complex (such as trying > multiple > iterations of hashing / stepping through the hash) seems way > over-engineered and > likely to induce error somewhere (I guarantee you that someone will forget > that > logic somewhere in server-side analysis code.) > > > > On 2012/01/25 22:04:45, jar wrote: > >> That (with a DCHECK or CHECK) was almost the previous CL upload. >> > > IMO, it is easier to just be dead right, and not have to explain to anyone >> (ever) that they need to change a name for a silly reason. It is (as you >> point out, for lack of an full iteration test), painful to test... and >> that >> complexity far exceeds the complexity of the tiny if/else/if code. >> > > Jim >> > > On Wed, Jan 25, 2012 at 1:58 PM, Alexei Svitkine >> > <asvitkine@chromium.org>wrote: > > > >> > >> > On Wed, Jan 25, 2012 at 4:03 PM, <mailto:ian@chromium.org> wrote: >> > >> >> The probability of ever hitting any of this code is so close to zero >> it's >> >> not >> >> even funny. It would be equally easy, IMO, to just not do any stepping >> at >> >> all >> >> and just have a DCHECK in there DCHECK(*bits) << "Choose a different >> name >> >> for >> >> field trial " << field_trial_name; >> >> >> >> and remove that code entirely. >> > >> > >> > I agree - especially if we can have a test that iterates all registered >> > field trials and calls this for every possible name / group_name. >> > >> > -Alexei >> > >> > >> > > > https://chromiumcodereview.**appspot.com/9117037/<https://chromiumcodereview.... >
How about this now? Thanks! BYE MAD https://chromiumcodereview.appspot.com/9117037/diff/14007/base/metrics/field_... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9117037/diff/14007/base/metrics/field_... base/metrics/field_trial.cc:196: static const size_t kStep = sizeof(uint32) / sizeof(char); On 2012/01/25 20:03:42, ian fette wrote: > A related note is that you're currently stepping through 32-bits at a time to > avoid a reserved value - you could just as easily step through 8 bits at a time, > thus giving you many more possible values from the 160-bit hash before you > exhaust it. Alexei's proposal would also step 4 bytes at a time. Although it > would lead to an unaligned memory access, given that you're doing a SHA-1 hash I > really don't think that would be significant. > > E.g. I don't understand why you need kStep to be 4 instead of 1. I personally > think this would be much more readable as: > > > uint32* bits = reinterpret_cast<uint32*>(sha1_hash); > size_t offset = 0; > while (*bits == kReservedHashValue && ++offset <= (kSha1Length - sizeof(uint32)) > { > bits = reinterpret_cast<uint32*>(sha1_hash + offset); > } > DCHECK(*bits == kReservedHashValue) << "PICK A BETTER NAME FOO!"; > > On 2012/01/25 18:59:39, Alexei Svitkine wrote: > > You can make this logic a little simpler - if you initialize |bits| to the > first > > uint32, then you can just increment it via |bits++| while checking bits < > > &sha1_hash[kSHA1Length]. > Went back to previous version... https://chromiumcodereview.appspot.com/9117037/diff/14007/base/metrics/field_... base/metrics/field_trial.cc:199: bits = reinterpret_cast<uint32*>(&sha1_hash[offset]); On 2012/01/25 19:52:14, jar wrote: > I suspect this code is not safe for big-endian vs little-endian machines. The > problem is only significant because we'll send this hash to an external server, > that has expectations about the relationship between the strings and its hash. > I asked around, and was told that we don't support big-endian. > > To be sure this works, please include a test case that has the expected hash > values for some known string. This way, if someone tried to port it, the test > would blow up, and identify the issue. Added tests... Good idea...
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/9117037/11015
Presubmit check for 9117037-11015 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: base/metrics/field_trial_unittest.cc,base/metrics/field_trial.cc,base/metrics/field_trial.h Presubmit checks took 1.5s to calculate. Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
On 2012/01/26 04:45:04, Alexei Svitkine wrote: > LGTM Thanks Alexei... I hit the commit box now, if others disagree, let me know ASAP... BYE MAD
Oops... I didn't check for owners first... So I guess I'll wait for an OWNERS approval now... BYE MAD On Thu, Jan 26, 2012 at 10:21 AM, <commit-bot@chromium.org> wrote: > Presubmit check for 9117037-11015 failed and returned exit status 1. > > Running presubmit commit checks ... > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for: > base/metrics/field_trial_**unittest.cc,base/metrics/** > field_trial.cc,base/metrics/**field_trial.h > > Presubmit checks took 1.5s to calculate. > > Was the presubmit check useful? Please send feedback & hate mail to > maruel@chromium.org! > > > http://codereview.chromium.**org/9117037/<http://codereview.chromium.org/9117... >
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/9117037/11015
Try job failure for 9117037-11015 (retry) on win_rel for step "browser_tests". It's a second try, previously, steps "unit_tests, browser_tests, ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
