Acb mods

[jimkring]

Thanks for these great contributions, @mcduff14. They are all very thoughtful and well documented.

Regarding the inlining of trim whitespace, I'm wondering if it makes sense to put this in a special "Trim Whitespace - Inlined.vi" subVI that's marked for inlining, rather than inlining at edit time (which, arguably, makes the code less maintainable). Is there a significant memory hit for the subVI instances of Inlined subVIs?

[mcduff14]

I assume that inline would probably work well and be more maintainable. I inlined it because of the comment on the "Error Cluster From Error Code.vi" along with some comments from AQ who I believe said the inlined trim whitespace is used so much that it would use too much memory if it was reentrant. I have no idea when that comment was made for the "Error Cluster From Error Code.vi" or if it is even still applicable.

​

On Sat, Jun 3, 2017 at 12:08 PM, Jim Kring notifications@github.com wrote:

Thanks for these great contributions, @mcduff14 https://github.com/mcduff14. They are all very thoughtful and well documented.

Regarding the inlining of trim whitespace, I'm wondering if it makes sense to put this in a special "Trim Whitespace - Inlined.vi" subVI that's marked for inlining, rather than inlining at edit time (which, arguably, makes the code less maintainable). Is there a significant memory hit for the subVI instances of Inlined subVIs?

[image: 2017-06-03_11-05-26] https://cloud.githubusercontent.com/assets/381432/26755949/c59d81ca-484c-11e7-9c99-d4e224b76d76.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JKISoftware/JKI-State-Machine/pull/3#issuecomment-305991803, or mute the thread https://github.com/notifications/unsubscribe-auth/AbuXdtM6hI01PQ18IKFj1_u3qYSzaTlWks5sAaEVgaJpZM4Nu1VK .

[jimkring]

Regarding "Error Cluster From Error Code.vi," since it's only trimming trailing whitespace (not leading and trailing whitespace), there's a performance benefit to only use the half that's needed. I'm wondering if that's why it was inlined (only using half of the function), as opposed to the benefits of just generally inlining the code altogether.

[mcduff14]

No idea. I am by no means a LabVIEW guru like yourself. I assumed the performance hit came from possibly waiting for a non-reentrant vi. Not using the first part of the Trim Whitespace function is just a conditional, which should be relatively fast, I think. Once again this comment may have been written 20 years ago and is not applicable anymore. My modifications were such that there were no blocking functions as I want all my loops to be able to run without waiting.

Cheers, mcduff

On Sat, Jun 3, 2017 at 1:47 PM, Jim Kring notifications@github.com wrote:

Regarding "Error Cluster From Error Code.vi," since it's only trimming trailing whitespace (not leading and trailing whitespace), there's a performance benefit to only use the half that's needed. I'm wondering if that's why it was inlined (only using half of the function), as opposed to the benefits of just generally inlining the code altogether.

[image: image] https://cloud.githubusercontent.com/assets/381432/26756448/6993e0e4-4857-11e7-807b-ae099a90d572.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JKISoftware/JKI-State-Machine/pull/3#issuecomment-305997324, or mute the thread https://github.com/notifications/unsubscribe-auth/AbuXdvwXQZDF1rIEWdEqPD4u2pen2NrGks5sAbhmgaJpZM4Nu1VK .

[jimkring]

I found another nice little improvement that shaved a non-trivial amount of execution time..

[mcduff14]

Good Catch!! I am implementing the change in my version.

On Sun, Jun 4, 2017 at 5:49 PM, Jim Kring notifications@github.com wrote:

I found another nice little improvement that shaved a non-trivial amount of execution time..

[image: 2017-06-04_16-44-15] https://cloud.githubusercontent.com/assets/381432/26766387/b5f22830-4945-11e7-9fd1-69fe6821ce12.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JKISoftware/JKI-State-Machine/pull/3#issuecomment-306075745, or mute the thread https://github.com/notifications/unsubscribe-auth/AbuXdgz55A9gAfi8WIFiG7YI-6aw5UTLks5sA0KAgaJpZM4Nu1VK .

[mcduff14]

You also cleaned up the comment loop, nice work.

On Sun, Jun 4, 2017 at 5:49 PM, Jim Kring notifications@github.com wrote:

I found another nice little improvement that shaved a non-trivial amount of execution time..

[image: 2017-06-04_16-44-15] https://cloud.githubusercontent.com/assets/381432/26766387/b5f22830-4945-11e7-9fd1-69fe6821ce12.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JKISoftware/JKI-State-Machine/pull/3#issuecomment-306075745, or mute the thread https://github.com/notifications/unsubscribe-auth/AbuXdgz55A9gAfi8WIFiG7YI-6aw5UTLks5sA0KAgaJpZM4Nu1VK .

[mcduff14]

One more optimization following your logic.

The trim whitespace for the string for the ">>" matches only needs tabs and spaces removed for trim whitespace. Since it is the first line no need for \n and \r characters. ​

[mcduff14]

On second thought, on the remaining lines after the EOL what is someone had

Command: State1 \s\s\t\n\r Command: State 2

The \n and \r would be missed, but I think they can be removed for the first line case.

On Sun, Jun 4, 2017 at 5:49 PM, Jim Kring notifications@github.com wrote:

I found another nice little improvement that shaved a non-trivial amount of execution time..

[image: 2017-06-04_16-44-15] https://cloud.githubusercontent.com/assets/381432/26766387/b5f22830-4945-11e7-9fd1-69fe6821ce12.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JKISoftware/JKI-State-Machine/pull/3#issuecomment-306075745, or mute the thread https://github.com/notifications/unsubscribe-auth/AbuXdgz55A9gAfi8WIFiG7YI-6aw5UTLks5sA0KAgaJpZM4Nu1VK .

[jimkring]

I created a unit test against the "Command: State1\s\s\t\n\rCommand: State 2" string, so that we can be sure to catch this case.

[drjdpowell]

When I stole your “State Queue” design for the “Action Stack” in “Messenger Library” I wrapped the “Parse State Queue” equivalent in an inlined wrapper that tests against an empty Queue (avoiding the call entirely when empty). A significant fraction of calls (at least in my use) are on an empty Queue, so this can be a significant improvement.

[mcduff14]

Am I missing something but can the Trim Trailing Comments be reduced to the following?

You can also get rid of String Subset function. The Match Pattern will return either an empty string, a string that has no comments, or the part of the string preceding the first comment. The Shift register in the loop takes care of everything, I think.

mcduff

[jimkring]

Yes, but I found that a Regular Expression (Match Pattern) search was less performant than a simple Search String.

[jimkring]

I stole your “State Queue” design...

@drjdpowell that's a great tip! you've redeemed yourself :-))

[jimkring]

@drjdpowell wow! that's a huge performance improvement. it takes nearly no time at all to execute. (BTW, I tried using the "Empty String/Path?" primitive and wiring the Boolean output into the Case Selector and it's much slower that wiring into the Case Selector as you've done)

[jimkring]

@drjdpowell I implemented a similar tweak in the Add State(s) primitive and it also makes a significant performance improvement in cases where the input strings are empty:

[mcduff14]

I am not seeing the same performance gains when adding the empty string check. It's possible that I am bench-marking incorrectly. My test consist of using a real application and looking at the Profile Performance and Memory. I am seeing gains at times, but the tool is probably imprecise at the microsecond level.

The following so far gives me the best performance for the Parse State, the Add State(s) primitive using @drjdpowell recommendation is sometimes faster sometimes slower.

Here's another option to test, I can't tell if it is faster, as of now my benchmark says about the same.

[jimkring]

@drjdpowell @mcduff14 I've created a Performance Optimizations branch and just made a beta build of the package. Please feel free to test and let me know if you see anything that looks like it needs some love https://github.com/JKISoftware/JKI-State-Machine/releases/tag/4.0.0.13-testing

[mcduff14]

Looks Good. These are possible issues for Parse State Queue: (See my comment beforehand, will post the pictures again.)

1. Only Match Pattern is needed, you do not need String length, String Subset, nor do you need to keep track of the match offset. Here's the logic: a.) Empty String - Match Substring Returns Empty String b.) Command with no comment - Match Substring Returns Command as that is before match c.) Command with one Comment - Match Substring Returns Command before match d.) Command with multiple Comments that are different - first iteration of match substring returns subset before before first match, if second comment if before first, then it will be caught on the next iteration, if it is before then it is already trimmed.

2.) The Trim Whitespaces should be modified for remaining lines and first line. For the remaining lines look for all characters both in front and back, that is, [\t\s\n\r]. It is possible the second line is \s\s\n. For the first line the trim whitespace should only have [\t\s] no possible way there is a \n or \r, since it is the first line. It is also possible to reduce the number of Match Patterns.

3.) Change the Error case Structure to a Select Case, it should be faster for the majority of cases. For the "rare" cases of an error, then slower, but the Error Case should be treated as a rare Poisson-like event.

Picts from before

Alternative Method less Pattern Matches

Edit: Forget about using the Select Function for the Error Case, it will give incorrect results for the remaining states.

[mcduff14]

One more possible optimization and issue for Parse State Queue.

Issue 1.) Change the Match Pattern for the last comment to "/\*" instead of "/" (Use only one asterisk, can't get one to appear in comment)

Optimization 1.) Change the Match Pattern vi in the Comment loop to Search/Split String vi. Do not use Regular Expressions with the Search/Split String vi. The Search/Split appears to be 33% faster on my benchmark than the Match Pattern vi. See picture below, also keep /* as it is for the Search/Split.

[jimkring]

@mcduff14 Thanks for these extra thoughts. Some comments on my findings:

Regarding the Search/Split String for comment removal, I found that the more complex solution of keeping track of the index and doing the actual splitting outside the loop was much faster.

This probably has to do with in-placeness. The input string buffer only has to be resized once at the end rather than on each loop iteration. This is really to do with how LabVIEW's compiler optimizations work.

Regarding the parsing of the ">>" arguments, I found that the 3 Match Pattern solution you posted was about ~20% faster! Nice work! 👍

Regarding the Error Handling with Select instead of Case Structure (as you showed below):

I found that you actually need three select nodes: (1) for state (empty string on Error) (2) for arguments (should be empty string on Error) and (3) for remaining states (should be preserved on error).

When I ran my benchmarks on this (3 Select nodes vs Error Case Structure) I found that it was slower (by about 10% or so).

I've committed my changes to the Performance Optimization branch and built a pre-release package, here:

https://github.com/JKISoftware/JKI-State-Machine/releases/tag/4.0.0.14-testing

[mcduff14]

Thanks for the in-depth explanations. Cheers, mcduff

[mcduff14]

First of all I would like to thank you for all your modifications and help and releasing the State Machine as open source, it has been a big help to my work. The only issues I see so far in 4.0.0.14 are

1.) Due to your earlier explanation for comment removal, remove the shift register

2.) This does not really matter because the next time it is parsed you will still get the correct answer, it just depends if you want to do it now or later. (The Trim Whitespaces could be modified for remaining lines to look for all characters both in front and back, that is, [\t\s\n\r].)

Cheers, mcduff

[jimkring]

You're right. I left that shift register because I thought I tested it and it was more performant than without it -- I was wrong.

I just tested after removing the shift register (as your recommend) and it shaves another 10% (3ms out of 30ms) for my Parse State Queue test case.

You're welcome. We're in a groove here and doing some good work :-)

[mcduff14]

Last Suggestion, one is style, the other may affect performance but probably not.

First Comment: 1.) In the "UI: Front Panel State" in the State Machine Template, change the case selector attached to the argument to "Case Insensitive Match". 2.) Rather than use the not equal boolean, wire the FP.State directly to case selector, no comparison needed. (Style)

Last Comment which may be meaningless and voodoo, but I follow: 1.) The comment below is buried deep in Simple Error Handler. This is the comment

Why does this subVI exist?

The property nodes for VI Server (used to programmatically control VIs and the value of controls) execute in the User Interface (UI) thread. An optimization in LV's compiler causes a case structure to execute entirely in the UI thread if any case of the case structure uses the UI thread and nothing in the other cases is explicitly unable to run in the UI thread. The General Error Handler.vi wants to avoid switching into the UI thread unless it absolutely has to. This allows the GEH to be used inside time critical loops without introducing thread synchronization. By moving this value property node into a subVI, the compiler optimization does not apply, and the GEH will only trigger the UI thread when it actually has to throw a dialog or when a string control reference is actually passed in.

So for anything with a reference, I generally put it in its own VI. So I usually have a subVI for this Front Panel State. I will try to add to the other files I uploaded. It is called WindowOpenClose and follows your "UI: Front Panel State" mods.

[mcduff14]

Will try to attach VI to this email.

On Tue, Jun 6, 2017 at 8:09 PM, Jim Kring notifications@github.com wrote:

You're right. I left that shift register because I thought I tested it and it was more performant than without it -- I was wrong.

I just tested after removing the shift register (as your recommend) and it shaves another 10% (3ms out of 30ms) for my Parse State Queue test case.

You're welcome. We're in a groove here and doing some good work :-)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JKISoftware/JKI-State-Machine/pull/3#issuecomment-306665626, or mute the thread https://github.com/notifications/unsubscribe-auth/AbuXdnDMXfkq0NYZC4Sm1hxJp38TNRBsks5sBgZbgaJpZM4Nu1VK .

[jimkring]

Hi @mcduff14. We've changed our Contributor License Agreement process. Can you please sign the CLA here: https://cla-assistant.io/JKISoftware/JKI-State-Machine?pullRequest=3

[jimkring]

@mcduff14 I'm going to close this pull request. We've incorporated a lot of these great ideas in the JKI State Machine 2018 release. Thank you for your great ideas and contributions!