asbo Posted January 26, 2012 Report Posted January 26, 2012 Re-saved in LV2009. OpenG LVOOP Suggestions.LV2009.zip
jgcode Posted January 29, 2012 Report Posted January 29, 2012 Here is my 2 cents: Firstly thanks, James these Vis are really cool - and I really dig the icons you did I have reviewed and attached the set of VIs and created Tests for all of them Open the project and run Main - this is kinda how it will plug into the current OpenG Test Framework I've included the help and BD as images so peeps can comment without downloading if preferred In general: Normally OpenG functions are not set to be re-enterent unless there is a specific need therefore, I have turned this setting off as it makes it easy for end users to debug plus there are associated memory costs with launching clones I don't think any of these VIs should output an error - it should be handled internally Documentation updated Additionally I have converted the VIs to OpenG style standards including: Removed terminals as icons Default 2009 panel color License (added a place holder - just ignore for now) Please comment or post on the attached VIs not on 2011 ones. Get Default Object I think we should leave in the disabled solution for knowledge and for a possible future review - I like that Default Value I don't see the need to pass the Object back out. I would assume this may help memory allocation but not 100% sure (can anyone comment). Regardless other comparison functions (both OpenG and LabVIEW) don't do this Is Same Or Descendant Class Change name to include Is prefix as per other comparison functions Again - I don't see the reason to pass out the Objects As we know about one Object but we are testing for the a possible descendant (as per name of output and VI) I think the relative names should be changed around (use Descendant instead of Ancestor) - this matches the output I changed the icon and placed the ? near the object that is being tested/checked to match the above point Return Class Name I don't see the benefit of having two VIs (originally Class Display Name and Qualified Class Name) and maintaining two similar pieces of code - so I merged the two I also changed the name to describe what the VI is doing (e.g. return or maybe resolve etc...) I removed the subVI call to the Get Default Object - it just wraps a prim so it's not a reuse issue (and it's not inlined so I assume this would remove overhead) I have added a summary of AQ's post into the VI documentation as well as a line of text from him regarding use as he provided the original code - LVOOP Data 2009.zip
drjdpowell Posted January 29, 2012 Author Report Posted January 29, 2012 (edited) I don't see the need to pass the Object back out. I would assume this may help memory allocation but not 100% sure (can anyone comment). Regardless other comparison functions (both OpenG and LabVIEW) don't do this Hi JG, great work! Please see my previous post where I introduced the object outputs. At least when calling these VI’s in a For Loop on an array of objects, the lack of an output seems to require a copy of the object, leading to poor performance for large objects. This adds (in my tests) more than 150 microseconds of execution time for objects containing a 1 million U8 byte array. This compares with times of between 1 and 10 microseconds for the various proposed VIs with object outputs (note, I don’t even need to connect the outputs). Admittedly, I don’t particularly like having outputs, especially on comparison VIs, but the performance difference is potentially large. Large enough for me to have to worry about it and have custom “high-performance" versions of the VI’s to use if it matters, or just use the raw code in each case. This would eliminate most of the advantages of putting these VIs in OpenG. You did a similar thing in replacing the “Get Default Object” subVI in the Name VI; I want to avoid that kind of thing as much as possible. Note, also, that having object outputs, though not consistent with other OpenG VIs, is consistent with how LVOOP methods are written, where even “Read” methods have an object output. So, I would strongly argue for these outputs, and encourage any lurkers to comment on this one way or the other. I’ll write more later, gotta go... — James Edited January 29, 2012 by drjdpowell
jgcode Posted January 29, 2012 Report Posted January 29, 2012 Please see my previous post where I introduced the object outputs. At least when calling these VI’s in a For Loop on an array of objects, the lack of an output seems to require a copy of the object, leading to poor performance for large objects. This adds (in my tests) more than 150 microseconds of execution time for objects containing a 1 million U8 byte array. This compares with times of between 1 and 10 microseconds for the various proposed VIs with object outputs (note, I don’t even need to connect the outputs). Sorry I missed that post above. As per my post I hadn't got to performance testing then, but I am doing some now as I have both VIs - the time difference is quite surprising - and I wonder if this would be optimized in the future? And what the answer to your question is regarding the effect of inlining. One thing I did notice is the coercion when there is no connecting output, which disappears when its thralled. ...is consistent with how LVOOP methods are written, where even “Read” methods have an object output. Well that depends who's code you read But yes, they should include the output given the results of the benchmarking
drjdpowell Posted January 29, 2012 Author Report Posted January 29, 2012 Normally OpenG functions are not set to be re-enterent unless there is a specific need therefore, I have turned this setting off as it makes it easy for end users to debug plus there are associated memory costs with launching clones How does the new “inline” option play in to that? In has always seemed strange to me that OpenG VIs are mostly non-reentrant. It makes them different from the LabVIEW primitives that they supplement/extend. But that might be a different conversation. Though I note that you did remove the “Get Default” subVI from “Return Class Name” to improve performance. In doing so you eliminated the self documenting advantage of the subVI; a new LabVIEW user will be mystified as to what “Preserve RunTime Class” is doing here. Would it not be better to “inline” that subVI and use it? Or just accept a performance hit (which I think is quite small)? At the very least we need a comment here. Side Note: I noticed that OpenG has an off-palette subVI for parsing a PString (“Get PString_ogtk”) that is used in the Variant VIs. Perhaps that should be used in “Return Class Name”, where we are walking along a set of PStrings? I don't see the benefit of having two VIs (originally Class Display Name and Qualified Class Name) and maintaining two similar pieces of code - so I merged the two The only advantage is getting the short name slightly faster, but that is probably not a good enough reason for a separate VI. BTW, maybe we should call the short name form “Base Name” or “Class Base Name”. Everything else looks fine to me. The only quibble is that the documentation for “Return Class Name” singles it out as being “highly inefficient” when it is considerably more efficient that many VIs that ship with LabVIEW, such as the VariantType Library or “Get LV Class Path”. But ignore the quibble. — James
jgcode Posted January 29, 2012 Report Posted January 29, 2012 How does the new “inline” option play in to that? It doesn't, or rather, cannot yet as the OpenG source is currently in 2009. Whenever it moves up where inlining is supported, then we will need to review it for valid use cases. Given you started in 2011 any information we have we should use to document the VIs now tho. In has always seemed strange to me that OpenG VIs are mostly non-reentrant. It makes them different from the LabVIEW primitives that they supplement/extend. But that might be a different conversation This has been discussed before (pretty sure on LAVA?) and I too was surprised by a) at the time I thought OpenG was all reentrant, and b) the evidence supplied I actually went ahead and converted all my reuse VIs back to non-reentrant (except where specifically required). Though I note that you did remove the “Get Default” subVI from “Return Class Name” to improve performance. In doing so you eliminated the self documenting advantage of the subVI; a new LabVIEW user will be mystified as to what “Preserve RunTime Class” is doing here. Would it not be better to “inline” that subVI and use it? Or just accept a performance hit (which I think is quite small)? At the very least we need a comment here. Noted, I can fix that by using documentation. Side Note: I noticed that OpenG has an off-palette subVI for parsing a PString (“Get PString_ogtk”) that is used in the Variant VIs. Perhaps that should be used in “Return Class Name”, where we are walking along a set of PStrings? I would prefer to group all the LVOOP functions together - so they are easy to find. Even though the implementation may be related, that could change in the future. It gets the Class Name from an Object so it should hang out with the other Object functions. The only advantage is getting the short name slightly faster, but that is probably not a good enough reason for a separate VI. BTW, maybe we should call the short name form “Base Name” or “Class Base Name”. Yep, that's the trade off but I prefer to lean over to maintainability here. Everything else looks fine to me. The only quibble is that the documentation for “Return Class Name” singles it out as being “highly inefficient” when it is considerably more efficient that many VIs that ship with LabVIEW, such as the VariantType Library or “Get LV Class Path”. But ignore the quibble. I will look at reviewing the grammar
PaulL Posted February 4, 2012 Report Posted February 4, 2012 Isn't the VI up above one that, given an object, returns the default value of that object? And Get LV Class Default Value.vi returns the default value if all you know is the class. What are you looking for? I've thought about this, and I wasn't asking for the correct thing--but there is a need that I still don't think LabVIEW fulfills--and that I view as quite important. It's not exactly a new issue. Rather than hijack this thread, I started a new thread here: http://lavag.org/topic/15440-object-serialization-xml-and-labview/
jgcode Posted February 5, 2012 Report Posted February 5, 2012 I am going to make these first class citizens of the lvdata package 1
asbo Posted February 5, 2012 Report Posted February 5, 2012 Man, that is a massive palette. Why does the it have its own version of the variant constant? If you eliminated that, you could condense it to a more reasonable 5x5.
Francois Normandin Posted February 5, 2012 Report Posted February 5, 2012 The native LabVIEW variant palette doesn't have a "Variant Constant". I personally find this one very useful.
jgcode Posted February 5, 2012 Report Posted February 5, 2012 Man, that is a massive palette. Why does the it have its own version of the variant constant? If you eliminated that, you could condense it to a more reasonable 5x5. I don't consider it 'that' big however, am happy to take on board such feedback (which is why I post stuff) - so I will see what I can do. I may move the constant but I will not remove it as Fraswa says - it is not native to LabVIEW and is handy to have.
Aristos Queue Posted February 5, 2012 Report Posted February 5, 2012 Hold off on finalizing this until the end of the week, please. I'm asking compiler folks to investigate the performance gain of adding the output terminal to see if there's another way to get that gain OR if it could at least be made unnecessary in LV 2012. Since this affects the conpane of the VIs, and that would be hard to remove in the future, it would be better to publish without it. If you don't hear from me by the end of this week, ping me, please. 2
jgcode Posted February 5, 2012 Report Posted February 5, 2012 Hold off on finalizing this until the end of the week, please. I'm asking compiler folks to investigate the performance gain of adding the output terminal to see if there's another way to get that gain OR if it could at least be made unnecessary in LV 2012. Since this affects the conpane of the VIs, and that would be hard to remove in the future, it would be better to publish without it. If you don't hear from me by the end of this week, ping me, please. Copy that. Very much appreciated too :beer:
jgcode Posted February 6, 2012 Report Posted February 6, 2012 Man, that is a massive palette. Why does the it have its own version of the variant constant? If you eliminated that, you could condense it to a more reasonable 5x5. It looks worse because of Icons and Text view - this is what it looks like as Icons only (in VIPM). I would like to be able to change this is VIPM - if anyone else does too then check out and vote for David's cool idea (and my comment!) on the VIPM Idea Exchange. I don't want to move stuff around too much due to palette inertia - but here is a second attempt for comment:
asbo Posted February 6, 2012 Report Posted February 6, 2012 The native LabVIEW variant palette doesn't have a "Variant Constant". I personally find this one very useful. I may move the constant but I will not remove it as Fraswa says - it is not native to LabVIEW and is handy to have. Ah ha! If my verbiage didn't make it obvious, I thought it was also somewhere else. You can see how often I've needed one. It looks worse because of Icons and Text view - this is what it looks like as Icons only (in VIPM). I don't want to move stuff around too much due to palette inertia - but here is a second attempt for comment: It does look much better in Icon view (which is what I use anyway, now that I pay attention to what I'm saying). The reorganization does help, either way. 1
Aristos Queue Posted February 7, 2012 Report Posted February 7, 2012 (edited) Victory! That output terminal will not be needed as of LV 2012. There was a (bug / unimplemented optimization -- depends upon your view) that made LV add a copy at certain upcast coercion dots. This compiler (fix / upgrade) will have performance benefits for a wide range of LVOOP applications. The change was implemented months ago, so this API doesn't get credit for finding the issue, but the compiler team verified your VIs do benefit from the change. I would therefore recommend not putting it into the API as removing the terminal is a possible mutation issue, and not having the output terminal for a read-only operation is a much better API overall. Edited February 7, 2012 by Aristos Queue 2
drjdpowell Posted February 7, 2012 Author Report Posted February 7, 2012 Oh good! Having to put those terminals in was annoying. I second the suggestion to go without the terminals. Though we might want a temporary note in the VI descriptions and/or on the block diagrams explaining this issue. — James
Aristos Queue Posted February 7, 2012 Report Posted February 7, 2012 This same change will have effect of removing a copy on control refnums and .NET refnums also, although the effect will be much much smaller since the maximum amount of data copied in those cases is a single int32.
asbo Posted February 8, 2012 Report Posted February 8, 2012 While on the topic of OpenG pallettes, does anybody know an easy way to get rid of the "OpenG.." text at the bottom of all of my pallette icons? It never used to be like this... My prefs are set to Category (standard) and all the other sub-pallettes do not have any text at the bottom. This started to happen a while (a year?) ago? The most immediate downside I see to this is that if you use Quick Drop or the search functionality, you won't be able to see whether the VI in question is OpenG or not. Whether or not this is catastrophic is up for debate. /derailment encouraging
jgcode Posted February 29, 2012 Report Posted February 29, 2012 Thanks everyone for their input - especially James for his donation of VIs. This OpenG Review is now closed. Please start a new thread to discuss new changes to this VI. Please PM me if there are any issues with this thread.
Recommended Posts