More refactoring

[a-ovchinnikov]

Tests added, code refactored some more. Parsers are updated and cleaned up.

[epcim]

@AndrewPickford - any objections?

[AndrewPickford]

The non parser related changes should be fine. The parser usage changes in reclass/values/parser.py and the knock on changes to reclass/reclass/values/parser_funcs.py have a problem.

Changing to use a single parser by default in reclass/values/parser.py lines 32 to 35 will incur a large speed penalty. The full parser (the object returned by get_ref_parser in the non refactored code) is very slow. This is the reason for using the $ test in the non refactored Parse class parse method and for having a second simplified parser that only deals with strings containing a single reference. The slow down is fine for testing but for nodes with large numbers of complex classes it's bad and for nodes with inv queries the slow down is even worse.

For this refactoring I would not merge the parser changes. Instead split them off and tackle them in smaller chunks - separate the parser changes into the simple format changes (like removing underscores) and then the logic changes to the parser. I'd advise this for both the parser and the interpolation done in the Parameters/Exports classes as they are both nastily complex areas of the reclass code.

[epcim]

I agree, let's split it into multiple patches and for the patch to parser check performance. Thanks

[epcim]

Alex, any idea when you will have time to split to "syntax" update and the part that touch the parser only. I would like to avoid conflicts, as they will always appear as we add new features.

[a-ovchinnikov]

Petr, I think, I will have time to address comments and suggestions in the next few days.

@AndrewPickford - could you please share data which shows that regular parser incurs large speed penalty? My tests show that it is just about two times slower than simple one, but I have to admit that I benchmarked them on synthetic input only.

[AndrewPickford]

@a-ovchinnikov Checked the timing with my current setup. A factor of a little over 2 looks to be about right between the current code and just using the full parser. I thought it was higher, but your synthetic benchmark matches my setup timings for this.

That said for my slowest nodes reclass takes 11 seconds currently and increases to 24 seconds just using the full parser. With about a 10% measurement uncertainty on those numbers. The more simple node times go from 3-4 to 7-9 seconds. I think these times will be at the high end for reclass runs - I'm using git and even my simple nodes still have a fair bit of reclass config.

In terms of salt runs these times are a non issue. Doing a full state run over even one of my simple nodes takes around 30 to 60 seconds, so an additional few seconds from reclass would not be a problem. But running reclass on the command line should be as fast as possible, waiting those extra 3 to 12 seconds adds up.

Also the original reclass is a bit faster still, at least on inputs that both can interpret. Minimising any speed regression between the two was the initial reason for the speed ups and the simplified parser.

[a-ovchinnikov]

I separated general refactoring from parser changes.

[AndrewPickford]

Ran some tests on with my setup. The refactored code produced identical output to the current develop branch so all good there. However the slow down has gotten even worse for my more complex nodes. Simple nodes again take about twice as long, but the more complex nodes take about 5 times as long. Going from 3.5 to 17.9 seconds in one case and for the worst case from 10.9 to 52.9 seconds.

The tests we currently have aren't any good for timing tests, run_tests.py on my laptop takes about 2.8 secs for the refactored code and 2.2 secs for the develop branch. And there's at least a +/- 0.1 sec uncertainty on these numbers. If would help to have a speed test, but it's a fair bit of work to make something large and realistic enough without copying a real reclass setup which will usually have non public information in it somewhere.

[a-ovchinnikov]

The slowdown of run_tests.py is expected: it is a runner for unit tests, I am adding a dozen or so of new unit tests some of which take time to execute, thus in the end we get longer execution times for the runner.

However the slowdown for complex nodes is rather surprising given that this PR consists of mostly minor deduplication tweaks. I definitely need to run few more tests. Meanwhile could you please provide some details regarding how you measure this slowdown? Are the values you shared mean? What about deviations? What was the number of tests? Also could you please include hash of a commit for which you do the measurements, just to play it the safest way and ensure we are both measurng the same thing?

[AndrewPickford]

@a-ovchinnikov The issue with run_test.py for timing tests is that it isn't doing enough work for the timing to be meaning full. And as you say adding a more tests (a good thing!) slows things down anyway.

I'm doing simple timing tests just running reclass with time, so:

time reclass --nodeinfo node1 -r

Unless I get a strange or long result I'm only running a test once. I'm not doing anything statistical here - I'm just testing for large changes. From running these tests few times the lowest uncertainty on these sorts of simple tests is around 0.1 secs, for the short few second tests. Longer tests have more uncertainty but I don't have any firm numbers for this.

The results from a few days ago were done against current salt-formulas:develop branch (d1a099ab91a6d6feec7031a0d1aa16427da2d6d5) and your a-ovchinnikov:develop branch (bb76a54ff6aa3ad024ff8e8d21185ffe59911d97).

Looking at the changes to reclass/values/parser.py, you're calling parsers.get_ref_parser and parsers.get_simple_ref_parser on each call to the Parse.parse method. That's probably where most of the slow down is coming from.

[a-ovchinnikov]

@AndrewPickford thank you for detailed answer! Indeed a bug manifesting itself in constant recreation of parsers crept in, my latest change should fix that.

[epcim]

I am about to merge it, @AndrewPickford have you tested with the latest version? May you update your review. Thanks

[AndrewPickford]

Looks good now. Speed issue is fixed.