Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13887007:
Introduce RawVarData and associated classes for serializing PP_Vars (Closed)
|
Created:
7 years, 8 months ago by raymes Modified:
7 years, 8 months ago CC:
chromium-reviews, yzshen1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIntroduce RawVarData and associated classes for serializing PP_Vars
This provides a replacement for the inner class of SerializedVar for serializing a PP_Var. It achieves 2 purposes: 1) it refactors the design of SerializedVar::Inner which was growing increasingly messy (and was going to become nastier) 2) it provides support for serializing/deserializing dictionary and array PP_Vars. To serialize a PP_Var, first the transitive closure of that var is computed (all nodes referenced) and then each var in the transitive closure is written to the message. Some trickiness arises from the fact that a PP_Var can be either a primitive or a reference and references have to be maintained when transmitting over the wire. The comments in the header files provide a description of the approach in more detail.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194629
Patch Set 1 #Patch Set 2 : #Patch Set 3 : . #Patch Set 4 : #
Total comments: 45
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : . #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Total comments: 2
Patch Set 11 : . #Patch Set 12 : . #
Total comments: 18
Patch Set 13 : #
Total comments: 4
Patch Set 14 : #Patch Set 15 : . #Patch Set 16 : . #
Messages
Total messages: 16 (0 generated)
I am still writing unittests for this but I have put it up so you can make comments on the design. It is not hooked up to anything in this CL but hooking it up to SerializedVar should be trivial. Thanks!
https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:29: const VarGraph* graph, Why not const&? https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:54: PP_Var ReferenceToVar(const RawVarData::VarReference& ref, "Reference" is a little overloaded, given these are reference-counted, and it makes a name like ReferenceToVar a little confusing to me. Maybe VarReference should be something like VarHandle? https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:73: RawVarData::VarReference result; Given VarReference has no default constructor, everything inside goes uninitialized. I think you want to either add a default constructor or use "= {}" everywhere you make one of these. I think the style guide would recommended a default constructor. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:87: PP_Var MakeInvalidRef(PP_VarType type) { I don't think we should even serialize invalid vars, so maybe you can get rid of this. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:145: data_.push_back(CreateRawVarData(var, NULL, instance)); Why do you need the if at all? (Notes to self: Looks like it's because if the var is a container, the root will end up in the graph... so adding the root and the contents of the graph could give a duplicate? But you could see if the graph is empty to decide if you need to add the root. Or... why does VarGraph not have all the values, too? That would simplify things here. What about elsewhere?) https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:165: data_[i]->InitPPVar(graph[i], graph); I found InitPPVar a bit confusing as a name. Maybe PopulateVarFromRawData, or something? Maybe it would be more obvious if it took a Var* out-param instead of a PP_Var? https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:200: result->data_.push_back(BasicRawVarData::Read(var_type, m, iter)); Random idea, not sure how hard this is... It would be nice if there was a way to restructure to have only one factory function that does the switch statement to var type to a specialization of RawVarData. I.e., make something like CreateRawVarData that's more general purpose and makes an empty RawVarData. Then the code here could be more like: result->data_.push_back(CreateRawVarData(var_type)); result->data_.back().Read(m, iter); without the whole switch thing. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:302: data_ = *string_var->ptr(); It's worth thinking about if there are ways to reduce copies. Our strings are immutable, so keeping scoped_refptr<StringVar> instead of std::string might be interesting. I think we'd have to change Var's base class to RefCountedThreadSafe, though. I think what you have models the current implementation. So that's probably the right first approach... https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:311: return StringVar::StringToPPVar(data_); I think you should use StringVar::SwapValidatedUTF8StringIntoPPVar instead to avoid this copy. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:320: m->WriteInt(PP_VARTYPE_STRING); It feels a little odd that the Write virtual function writes the type, but Read does not read it. Maybe each child of RawVarData should _not_ write its own type? Do it at the caller site instead, like for Read? https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:338: return new StringRawVarData(valid_var_ref, data); I can't see where this constructor is actually implemented...? Seems like you could use data as an in-out param and swap() it inside that constructor to avoid a copy. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:376: buffer_var->ByteLength()); Same comment as for String... seems like we could avoid this copy. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:420: static_cast<uint32>(data_.size()), data_.data()); On the plugin side, if data_ is a vector<uint8_t>, you can provide a swapping implementation to avoid this copy. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:503: data_.push_back(VarToReference(iter->get(), graph)); It seems strange to me that you have two different places where you loop over each Array and each Dictionary. Let me see if I understand... you do it once upfront in order to build your "VarGraph", which really just gives you a way to find when a ref-counted Var appears more than once. *Then*, you loop over each one again to populate the actual "RawVarData"s. Why is that necessary? It seems like you could avoid iterating twice. It seems potentially expensive for large objects. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.h File ppapi/proxy/raw_var_data.h (right): https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.h... ppapi/proxy/raw_var_data.h:64: virtual ~RawVarDataGraph(); I don't see you using this as a base class, so it seems like this shouldn't be virtual. Am I missing something? https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.h... ppapi/proxy/raw_var_data.h:95: // along with the graph it is apart of to be initialized. apart->a part https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.h... ppapi/proxy/raw_var_data.h:96: virtual void InitPPVar(const PP_Var& var, If CreatePPVar returns |var|, it seems a little odd to pass it back in? Should we store the scoped_refptr<Var> so we don't need that? Or have a Var out-param (Var*)? https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.h... ppapi/proxy/raw_var_data.h:109: bool valid_var_ref; Not sure we need this... if any are invalid, maybe we should just bail? Also doesn't make sense for value types. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.h... ppapi/proxy/raw_var_data.h:192: std::string data_; vector<uint8_t> seems more natural to me. https://codereview.chromium.org/13887007/diff/7001/ppapi/shared_impl/var_grap... File ppapi/shared_impl/var_graph.cc (right): https://codereview.chromium.org/13887007/diff/7001/ppapi/shared_impl/var_grap... ppapi/shared_impl/var_graph.cc:32: var_graph->nodes.push_back(current); Why have the vector at all? You could just stick PP_Vars in the map. https://codereview.chromium.org/13887007/diff/7001/ppapi/shared_impl/var_graph.h File ppapi/shared_impl/var_graph.h (right): https://codereview.chromium.org/13887007/diff/7001/ppapi/shared_impl/var_grap... ppapi/shared_impl/var_graph.h:19: // A VarGraph is an intermediate representation of a graph of PP_Vars which is nit: is is https://codereview.chromium.org/13887007/diff/7001/ppapi/shared_impl/var_grap... ppapi/shared_impl/var_graph.h:34: struct VarGraph { This isn't really a Graph, best I can tell. It seems to just hold the nodes of the graph, but has no edge information. Maybe VarGraphNodes or something? https://codereview.chromium.org/13887007/diff/7001/ppapi/shared_impl/var_grap... ppapi/shared_impl/var_graph.h:40: std::map<int64_t, size_t> id_map; hash map is probably a better choice for int64_t
Forgot general comments: I'm not sure I totally understand what I'm looking at yet :-) But it looks a lot like an adaption of structured clone to Pepper, except that I think you have a two-phase process (put all the nodes in a map, then actually write them out). It might be cleaner if it was a one-phase algorithm. But I might be totally off-base.
https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:503: data_.push_back(VarToReference(iter->get(), graph)); Sort of. The VarGraph gives you 2 things: 1) The transitive closure of nodes (which is the part about finding whether a var appears more than once). 2) Translates the mapping of var id's. That is, it converts a global PP_Var as_id value into a local index in the transitive closure. This is needed because when you serialize you want to know which var in the transitive closure is being referenced by a var in a container. Looping over twice is necessary I believe. At the point where you iterate over the elements of the array in the VarGraph, you don't know what index those elements will be written to in the message. Basically the VarGraph attributes an index in the message to the data associated with each Var. The "data" associated with an ArrayVar are these local indices into the array, so they all need to be computed first. However, in my current code I actually do iterate an unecessary number of times because I iterate over each element once when an ArrayVar is created and then once again when it is written or read. I think this results in a much cleaner design though.
> But it looks a lot like an adaption of structured clone to Pepper, except > that I > think you have a two-phase process (put all the nodes in a map, then > actually > write them out). It might be cleaner if it was a one-phase algorithm. But I > might be totally off-base. Yes - it is sort of 2 phase in that I compute the transitive closure first and then write them out. However, I *believe* this is necessary if we want an iterative algorithm (note that the algorithm specified on the w3 page is recursive). Consider converting an array PP_Var to any other format. You would start by examining each element of the array. You can't directly convert each element at that time (because that would be recursive) so you push them on to the stack and convert them later. Now you have the problem that you can't fully convert the array. You have to revisit it later once all the elements in the transitive closure have been converted. The fact that I have pulled out the part about computing the transitive closure into VarGraph was purely a design choice though (because I could forsee reusing it with other conversions). I could always get rid of that class and move the code into the RawVarDataGraph class. It wouldn't change much though. Hopefully this makes sense - maybe I'm missing something :) Let me know your thoughts. On Fri, Apr 12, 2013 at 3:41 PM, <dmichael@chromium.org> wrote: > Forgot general comments: > I'm not sure I totally understand what I'm looking at yet :-) > > But it looks a lot like an adaption of structured clone to Pepper, except > that I > think you have a two-phase process (put all the nodes in a map, then > actually > write them out). It might be cleaner if it was a one-phase algorithm. But I > might be totally off-base. > > https://codereview.chromium.org/13887007/
(btw both the previous responses basically address the same issue) On Fri, Apr 12, 2013 at 4:34 PM, Raymes Khoury <raymes@chromium.org> wrote: >> But it looks a lot like an adaption of structured clone to Pepper, except >> that I >> think you have a two-phase process (put all the nodes in a map, then >> actually >> write them out). It might be cleaner if it was a one-phase algorithm. But I >> might be totally off-base. > > Yes - it is sort of 2 phase in that I compute the transitive closure > first and then write them out. However, I *believe* this is necessary > if we want an iterative algorithm (note that the algorithm specified > on the w3 page is recursive). > > Consider converting an array PP_Var to any other format. You would > start by examining each element of the array. You can't directly > convert each element at that time (because that would be recursive) so > you push them on to the stack and convert them later. Now you have the > problem that you can't fully convert the array. You have to revisit it > later once all the elements in the transitive closure have been > converted. > > The fact that I have pulled out the part about computing the > transitive closure into VarGraph was purely a design choice though > (because I could forsee reusing it with other conversions). I could > always get rid of that class and move the code into the > RawVarDataGraph class. It wouldn't change much though. > > Hopefully this makes sense - maybe I'm missing something :) Let me > know your thoughts. > > On Fri, Apr 12, 2013 at 3:41 PM, <dmichael@chromium.org> wrote: >> Forgot general comments: >> I'm not sure I totally understand what I'm looking at yet :-) >> >> But it looks a lot like an adaption of structured clone to Pepper, except >> that I >> think you have a two-phase process (put all the nodes in a map, then >> actually >> write them out). It might be cleaner if it was a one-phase algorithm. But I >> might be totally off-base. >> >> https://codereview.chromium.org/13887007/
Thanks for taking a look! I had some thoughts over the weekend based on your suggestions and made some changes to the design. I hope that it is simpler to understand now. It is still tricky though and I believe there are some unavoidable complexities. Don't bother looking at the unittest code yet. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:54: PP_Var ReferenceToVar(const RawVarData::VarReference& ref, This has all been removed now. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:73: RawVarData::VarReference result; " https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:87: PP_Var MakeInvalidRef(PP_VarType type) { Done - this is all removed. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:145: data_.push_back(CreateRawVarData(var, NULL, instance)); I changed the way this works, hopefully it is clearer now. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:165: data_[i]->InitPPVar(graph[i], graph); See my reply in the header. I changed the name as you suggested. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:200: result->data_.push_back(BasicRawVarData::Read(var_type, m, iter)); I think that's a good idea. Done. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:302: data_ = *string_var->ptr(); I haven't yet addressed any of these comments related to reducing copies yet. I'm going to wait until the high level design is right. Also it might be worth getting something comitted first and then optimizing later. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:311: return StringVar::StringToPPVar(data_); " https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:320: m->WriteInt(PP_VARTYPE_STRING); On 2013/04/12 22:40:38, dmichael wrote: > It feels a little odd that the Write virtual function writes the type, but Read > does not read it. Maybe each child of RawVarData should _not_ write its own > type? Do it at the caller site instead, like for Read? Done. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:338: return new StringRawVarData(valid_var_ref, data); " https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:376: buffer_var->ByteLength()); " https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:420: static_cast<uint32>(data_.size()), data_.data()); " https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.h File ppapi/proxy/raw_var_data.h (right): https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.h... ppapi/proxy/raw_var_data.h:64: virtual ~RawVarDataGraph(); On 2013/04/12 22:40:38, dmichael wrote: > I don't see you using this as a base class, so it seems like this shouldn't be > virtual. Am I missing something? Done. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.h... ppapi/proxy/raw_var_data.h:95: // along with the graph it is apart of to be initialized. On 2013/04/12 22:40:38, dmichael wrote: > apart->a part Done. https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.h... ppapi/proxy/raw_var_data.h:96: virtual void InitPPVar(const PP_Var& var, I debated about doing something like this be decided I would rather not store temporary data in the object because i think it can cause confusion. I do think the name is bad though and renamed the function to PopulatePPVar as you suggested :) https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.h... ppapi/proxy/raw_var_data.h:109: bool valid_var_ref; Done - if there are any invalid vars in the graph, I now just bail. https://codereview.chromium.org/13887007/diff/7001/ppapi/shared_impl/var_grap... File ppapi/shared_impl/var_graph.cc (right): https://codereview.chromium.org/13887007/diff/7001/ppapi/shared_impl/var_grap... ppapi/shared_impl/var_graph.cc:32: var_graph->nodes.push_back(current); This has changed. https://codereview.chromium.org/13887007/diff/7001/ppapi/shared_impl/var_graph.h File ppapi/shared_impl/var_graph.h (right): https://codereview.chromium.org/13887007/diff/7001/ppapi/shared_impl/var_grap... ppapi/shared_impl/var_graph.h:19: // A VarGraph is an intermediate representation of a graph of PP_Vars which is this is removed now https://codereview.chromium.org/13887007/diff/7001/ppapi/shared_impl/var_grap... ppapi/shared_impl/var_graph.h:34: struct VarGraph { This is removed now https://codereview.chromium.org/13887007/diff/7001/ppapi/shared_impl/var_grap... ppapi/shared_impl/var_graph.h:40: std::map<int64_t, size_t> id_map; On 2013/04/12 22:40:38, dmichael wrote: > hash map is probably a better choice for int64_t Done.
Thanks for taking a look! I had some thoughts over the weekend based on your suggestions and made some changes to the design. I hope that it is simpler to understand now. It is still tricky though and I believe there are some unavoidable complexities. Don't bother looking at the unittest code yet.
This is looking pretty good, I like what you've done. Thanks! https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/13887007/diff/7001/ppapi/proxy/raw_var_data.c... ppapi/proxy/raw_var_data.cc:503: data_.push_back(VarToReference(iter->get(), graph)); On 2013/04/12 23:21:38, raymes1 wrote: > Sort of. The VarGraph gives you 2 things: > 1) The transitive closure of nodes (which is the part about finding whether a > var appears more than once). > 2) Translates the mapping of var id's. That is, it converts a global PP_Var > as_id value into a local index in the transitive closure. This is needed because > when you serialize you want to know which var in the transitive closure is being > referenced by a var in a container. > > Looping over twice is necessary I believe. At the point where you iterate over > the elements of the array in the VarGraph, you don't know what index those > elements will be written to in the message. Basically the VarGraph attributes an > index in the message to the data associated with each Var. The "data" associated > with an ArrayVar are these local indices into the array, so they all need to be > computed first. > > However, in my current code I actually do iterate an unecessary number of times > because I iterate over each element once when an ArrayVar is created and then > once again when it is written or read. I think this results in a much cleaner > design though. > It looks like you figured out how to avoid the two phases. Thanks, looks cleaner to me. https://codereview.chromium.org/13887007/diff/33001/ppapi/proxy/raw_var_data.cc File ppapi/proxy/raw_var_data.cc (right): https://codereview.chromium.org/13887007/diff/33001/ppapi/proxy/raw_var_data.... ppapi/proxy/raw_var_data.cc:137: // Everything except the root will have one extra ref. Remove that ref. I'm surprised this is the case. I'm expecting they should have 1 per parent node (i.e., Var node A will have 1 ref-count for every Var node B that is a container with A as a child.) But I haven't looked at where this happens yet. So... general comment would be to please check for proper ref-counts in your unit test if you aren't already. https://codereview.chromium.org/13887007/diff/33001/ppapi/proxy/raw_var_data.... ppapi/proxy/raw_var_data.cc:608: } // namespace ppapi I notice that you deleted VarGraph and this file is still 43 lines shorter. Nice :)
Thanks. I've added tests and I think this is ready for a more detailed look when you have a chance. I haven't optimized any of the String operations yet though. Let me know if you think this is a showstopper otherwise I could add a TODO. > I'm surprised this is the case. I'm expecting they should have 1 per > parent node (i.e., Var node A will have 1 ref-count for every Var node B > that is a container with A as a child.) But I haven't looked at where > this happens yet. > > So... general comment would be to please check for proper ref-counts in > your unit test if you aren't already. This is the case because when you create a var, it has a refcount of 1 and then when you add it to containers, it gets additional references. You want the "root" var to keep this additional refcount (because it is like a handle) but other vars shouldn't otherwise they wont go away when the root is deleted. I made sure the unittest checks for this.
On Mon, Apr 15, 2013 at 6:24 PM, Raymes Khoury <raymes@google.com> wrote: > Thanks. I've added tests and I think this is ready for a more detailed > look when you have a chance. > > I haven't optimized any of the String operations yet though. Let me > know if you think this is a showstopper otherwise I could add a TODO. > I think the important thing is not to regress. I think that when we make a StringVar out of a RawVarData that we swap the string in, so I think you should do that here too. The rest can probably be later. > > > I'm surprised this is the case. I'm expecting they should have 1 per > > parent node (i.e., Var node A will have 1 ref-count for every Var node B > > that is a container with A as a child.) But I haven't looked at where > > this happens yet. > > > > So... general comment would be to please check for proper ref-counts in > > your unit test if you aren't already. > > This is the case because when you create a var, it has a refcount of 1 > and then when you add it to containers, it gets additional references. > You want the "root" var to keep this additional refcount (because it > is like a handle) but other vars shouldn't otherwise they wont go away > when the root is deleted. I made sure the unittest checks for this. >
Looking good. Are you planning on using this in SerializedVar in this CL, or do you want to defer that to a separate CL? https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... File ppapi/proxy/raw_var_data_unittest.cc (right): https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:48: base::hash_map<int64_t, int64_t>* visited_map) { May be worth a comment... it seems visited_map is not just a way to tell what's been visited, but also maps the source Var id to the target Var id after serialization/deserialization. (A.k.a. maps the id of a Var in |expected| to the id of its clone in |actual|). It took me a minute to figure out what was going on. https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:71: return expected.value.as_id == actual.value.as_id; This could be a DCHECK instead, right? https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:76: return expected_var == actual_var; Since we want to bail for invalid Vars, should this instead to a DCHECK or something? (similar for other refcounted types) https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:88: return expected_data == actual_data; Optional: It feels a little odd to copy them just to compare. You could do something like: if (expected_var->ByteLength() != actual_var->ByteLength()) return false; return memcmp(expected_var->Map(), actual_var->Map(), expected_var->ByteLength()) == 0); Granted, it's a test, so not that important to be optimized. https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:119: actual_iter != actual_var->key_value_map().end(); nit/suggestion: you don't really need to bounds check both iterators, since you know the maps are equal size. That might save you from the kind of weird formatting here. https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:134: } Optional suggestion: You might want to have logging for places where you detect inequality, to make debugging easier. Or not... somebody debugging a problem can do that later. https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:243: // Break the cycle. This is because we'll leak it otherwise? I feel like we should consider fixing that. IMHO, we already need to improve our VarTracker release code. I'm fairly sure recursively deleting a deep Var is going to cause stack overflows right now. Probably would only take a few hundred levels deep. But that should be a separate CL, in any case. Maybe a TODO here would suffice. https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:257: } How about a test where one direct child references another child, for both Dictionary and Array? E.g.: array->Set(0, release_array2.get()); array->Set(1, some_string_var.get()); array2->Set(0, some_String_var.get()); So not a cycle, but more like a diamond: Array1 ---------\ | | v v Array2 ----> String I'm pretty sure your code handles it right, but some implementations would handle cycles but not this.
Thanks for the quick feedback! I am planning on doing the plumbing to SeializedVar in a separate CL which I have coming up. https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... File ppapi/proxy/raw_var_data_unittest.cc (right): https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:48: base::hash_map<int64_t, int64_t>* visited_map) { On 2013/04/16 16:36:53, dmichael wrote: > May be worth a comment... it seems visited_map is not just a way to tell what's > been visited, but also maps the source Var id to the target Var id after > serialization/deserialization. (A.k.a. maps the id of a Var in |expected| to the > id of its clone in |actual|). It took me a minute to figure out what was going > on. Done. https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:71: return expected.value.as_id == actual.value.as_id; Not sure exactly what you mean? In the old code, the SerializedVar::Inner actual just transmitted the var ID. So I have kept the same behavior. This check verifies that case is handled correctly. https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:76: return expected_var == actual_var; Good idea this was a remnant from when I handled those cases. https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:88: return expected_data == actual_data; haha I avoid C memory functions like the plague and since it was a test I did the inefficient thing. However, since you provided the code, done :) https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:119: actual_iter != actual_var->key_value_map().end(); On 2013/04/16 16:36:53, dmichael wrote: > nit/suggestion: you don't really need to bounds check both iterators, since you > know the maps are equal size. That might save you from the kind of weird > formatting here. Done. https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:134: } Ok - it's a good suggestion but I just added a TODO for now. https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:243: // Break the cycle. Yes we leak otherwise. I agree. We need to provide something better for handling cycles. Added a TODO. https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:257: } Good idea. I also forgot to add a test for the simple case of referencing (for example) the same string twice in an array.
lgtm https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... File ppapi/proxy/raw_var_data_unittest.cc (right): https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:71: return expected.value.as_id == actual.value.as_id; On 2013/04/16 18:27:19, raymes1 wrote: > Not sure exactly what you mean? In the old code, the SerializedVar::Inner actual > just transmitted the var ID. So I have kept the same behavior. This check > verifies that case is handled correctly. You're right, nevermind. Was confused because we don't support PostMessage/HandleMessage for these. https://codereview.chromium.org/13887007/diff/42001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:88: return expected_data == actual_data; On 2013/04/16 18:27:19, raymes1 wrote: > haha I avoid C memory functions like the plague and since it was a test I did > the inefficient thing. However, since you provided the code, done :) I almost suggested std::equal, but you would have to cast the result of Map, which would be kind of annoying. Do whichever you like :-) https://codereview.chromium.org/13887007/diff/56001/ppapi/proxy/raw_var_data_... File ppapi/proxy/raw_var_data_unittest.cc (right): https://codereview.chromium.org/13887007/diff/56001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:48: // following reference cycles. It also ensures that id X in expected always maps to id Y in expected, right? Which seems like a good property. If you just did value comparisons all the way down, you might see two things as equal that contain the same data but are actually topographically very different. https://codereview.chromium.org/13887007/diff/56001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:210: Do you want to somehow clear the array out between tests? You're actually growing it with each new test. Seems okay, but what you're actually testing is a bit more than what the comment says.
Thanks! https://codereview.chromium.org/13887007/diff/56001/ppapi/proxy/raw_var_data_... File ppapi/proxy/raw_var_data_unittest.cc (right): https://codereview.chromium.org/13887007/diff/56001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:48: // following reference cycles. Yes exactly :) This was tricky to say concisely though but I think you did a good job so I adopted your wording :) https://codereview.chromium.org/13887007/diff/56001/ppapi/proxy/raw_var_data_... ppapi/proxy/raw_var_data_unittest.cc:210: Yeah the state builds up cumulatively. I'd prefer to leave it for better coverage for now. I think the comments are still accurate, like you said it just tests a superset of that. I guess if I were to be really thorough I would have tests for each of those things individually and then a complex test which mixes them.
Message was sent while issue was closed.
Committed patchset #16 manually as r194629 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
