CUSE performance improvement #519
Conversation
JamesWekel
requested review from
Fish-Git,
ivan-w,
Juergen-Git and
Peter-J-Jansen
as code owners
|
Thanks, James! I'll take a look at it and get back to you. |
|
Fish, Performance has improved, but I'm not exactly happy with the implementation. I optimized special cases which leads to a long implementation. The solution needs a second pair of eyes! Jim |
There was a problem hiding this comment.
I'm seeing some serious issues with your code, James. :(
The first thing I noticed was the following (similar code which appears in several other places):
/* update mainstore addresses; check for page boundary */
if ( (uintptr_t) m1 & PAGEFRAME_PAGEMASK )
m1--;
else
m1 = MADDRL((ea1 + (ss_scan_index-1)) & ADDRESS_MAXWRAP( regs ), 1, b1, regs, ACCTYPE_READ, regs->psw.pkey );Please explain how the above if statement is checking for whether a page is being, or is about to be, crossed?!
The PAGEFRAME_PAGEMASK constant is #defined (for e.g. z/Arch) as 0xFFFFFFFFFFFFF000ULL, which is the mask you apply to an address to get the start of the page (i.e. page number) for that address. Only in the highly unusual case where m1 was pointing to low core (i.e. to the very first page of storage, i.e. address 0) would that if statement ever be false! As coded, it is almost certainly always going to be true!
Another minor issue is you declare (and set) two variables called m1prior and m2prior, but never use them for anything.
Finally, you #define a constant called CUSE_INTERRUPT_POINT and an associated variable called ruptcnt and use them to decide whether or not the registers should be updated (with the comment update GPRs if scanned 1024 bytes - could get interrupt).
But yet you never bother to check for an interrupt anywhere (which is actually a good thing in this case because you shouldn't ever be checking for an interrupt!).
I suspect you read in the manual that CUSE was an interruptible instruction and so thought you should probably have code for that. But here's a hint: an instruction (in Hercules) can never be interrupted while it's executing! (unless you specifically do so of course, which you rarely, if ever, need to do.)
The only time you really need to MAYBE check for an interrupt is at the END of the instruction, once it has completed, and you're about to return from your function. For a good illustrative example of what I'm talking about, take a look at the code for the LPSWE instruction:
Lines 5219 to 5261 in d0ccfbc
Notice that it ends with a RETURN_INTCHECK(regs); statement. That causes an immediate jump to our instruction execution logic that checks for an interrupt for that CPU. Otherwise, when an instruction finishes executing, it simply returns, which goes on to the next instruction.
Checking for interrupts in Hercules is an expensive thing so we purposely don't do it after every instruction. Instead, we always execute a certain fixed number of instructions in a loop, and only check for an interrupt at the end of that loop:
Lines 2001 to 2024 in d0ccfbc
(Note: MAX_CPU_LOOPS is currently hard coded at 256)
I've temporarily stopped my review at this point in order to report on the above issues with your code. I have not finished with my review yet. For example, I have not reviewed your actual compare_until_substring_equal function yet, nor whether your helper functions are actually doing what they're supposed to be doing. I will get around to eventually doing that, and will report back to you with the results of my analysis/review once it's completed. In the mean time, I'm afraid I'm going to have to reject your pull request at this time until you can correct the issues I've already identified.
In order to help you, I would like to offer you the below patch for your consideration. You might consider applying it to your existing code, and then compare (review) the changes I've made to see whether you like them or not:
- CUSE.patch.txt (some proposed corrections)
NOTE! I'm a human being too and subject to making mistakes just like anyone else! The above patch could well contain serious bugs of its own! I don't believe it does (I believe it's good), but the burden of review falls squarely on your shoulders should you wish to use it!
I hope that helps you!
|
James (@JamesWekel), Additionally, one more extremely minor required change (in addition to the previously mentioned): For the Interestingly, even 300 seconds (5 minutes) isn't long enough for my system! But since I use a helper script which uses a runtest timeout factor of 2.0, I can manage to squeak by. When I tested current Hercules on my system (without your changes), here are the timings I got: (current unfixed Hyperion): So apparently your system is much faster than mine! Anyway... I hope I didn't scare you off this project with my previous critique of your code. You just need to make a few changes and you should be good to go. I finished reviewing the rest of your code (all of your helper functions as well as the primary CUSE instruction function itself too) and everything else looks good! So just make those suggested fixes I mentioned (about detecting when you've crossed over to a new page) and I'll be happy to accept your pull request. Thanks! |
|
Fish, Sorry for the late reply; busy with other life items. Thank you! Thank you! Thank you! How the (beep) (beep) did I code The current /* update GPRs if we just crossed half page - could get rupt */
if ((addr1 & 0x7FF) == 0 || (addr2 & 0x7FF) == 0)
{
/* Update R1 and R2 to point to next bytes to compare */
SET_GR_A( r1, regs, addr1 );
SET_GR_A( r2, regs, addr2 );
/* Set R1+1 and R2+1 to remaining operand lengths */
SET_GR_A( r1+1, regs, len1 );
SET_GR_A( r2+1, regs, len2 );
}and yes, I "read in the manual that CUSE was an interruptible instruction". So, it must be necessary but, with the optimizations, I would never know when a half page was crossed, therefore I just changed when the registers were saved. I now have some more reading on Hercules interrupts! Thank you for the CUSE patch. I do like the simpler look for page crossing checks. It is much more explicit in the code; not relying on a comment and how page offset bits are going to change when crossing a page. To make the code even more explicit, I've added the following macro #undef MAINSTOR_PAGEBASE
#define MAINSTOR_PAGEBASE( _ma ) ((BYTE*) ((uintptr_t) ( _ma ) & PAGEFRAME_PAGEMASK))to further simplify the look of crossed page checks. I'll do another review this afternoon. You should have a commit tonight. I've changed
Not at all. Thanks again for the review and the second pair of eyes, Jim |
It happens.
Yeah, I saw that myself too. But you need to keep in mind that Hercules's current implementation of a given instruction/function isn't necessarily "gospel" (sacrosanct). A good programmer should always question existing code by asking oneself "Is this right?", "Is this necessary?", "Is this the best way to do this?". As it turns out in this particular case (as I explained in my earlier comment) it's not actually needed. It's a complete waste of time. The only time an interrupt can occur is if/when the instruction purposely exits (ends), which is obviously not occurring in this case.
A basic understanding certainly wouldn't hurt, but it's not critical. I wouldn't recommend spending a lot of time on it unless you were interested in examining that part of Hercules's functionality. For most instruction coding however, it's not that critical. I only tried explaining it so you could see (understand) how silly and unnecessary such code was.
Good! I was slightly worried. Glad we're still cool.
COOL! I look forward to seeing it! You're doing great work, James! MUCH appreciated!
Not a problem. |
My system has 3.0 GHz Xeon X5570s. I have no idea which "generation" they are. (I don't concern myself with such things.) I think maybe the reason my numbers might be slower than yours is because:
I'm guessing you're using a much newer version of Visual Studio than me (2022? 2019?) and thus are using a version of Microsoft's compiler that is better able to leverage the newer instructions/features that your more modern processor has (whereas mine, having older processors and an older compiler, must resort to slightly less efficient instructions common with older processors). But that's just a guess. ONE of these days I'll upgrade my system to newer hardware and then maybe at that time I'll upgrade my Visual Studio as well. But right now I'm still poor and money is tight, so I'll probably be sticking with what I have for a few more years yet. (It's a nice system! I really like it! It's a Dell Precision T7500) |
|
You can compare almost any combination of CPUs on this site: https://cpu.userbenchmark.com/Compare/Intel-Xeon-X5570-vs-Intel-Core-i5-1135G7/m12230vsm1286124 Bill |
|
Fish, Let's try this again with a cross page fixes! For my current environment, I'm using a 'small' Intel NUC with Debian as a server with "gcc (Debian 10.2.1-6) 10.2.1" for Hercules development. I access the environment using Visual Studio Code with remote explorer from a really old Intel NUC with a slow dual core i3-4010U CPU running Windows 10. I have Visual Studio 2022 on the I3 NUC just to check that I haven't broken the VS project files. The server NUC is newer (faster CPU and memory) so that's probably where I'm getting better results. With the fixed cross page checks, the performance test now reports: Hope your final review doesn't find any more major errors! Jim |
Nope! Looks good to me! Pull Request accepted and merged! Thanks again, James! |
Fish,
Here is a proposed performance improvement for the CUSE instruction.
Before the change, the performance test reported:
and after the change:
The CUSE instruction performance is very dependent upon operand strings and substring length. CUSE-02-performance test shows performance increases between 95 - 99%. This increase is probably not representative of real-world instruction usage.
Thank you for your review and updates. I'm sure that there is something that I've missed.
Jim