-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CU14 - Performance Improvement #623
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm only getting started, and please don't take this the wrong way, because I realize I'm nit-picking here, but on line 2382 you have:
// /* Get mainstor address to Destination byte */
I'd appreciate the removal of the //.
Other than that, as far as your logic goes, it looks perfect to me! Simple. Straightforward. Efficient. Well done! I like it. :)
I'll take a look at your two tests next (CU14-01 and CU14-02).
For now, just make that one teensy change and you're golden.
p.s. I just made two minor commits today too, so you might want to do a refresh to pick them up. No big deal as they're completely unrelated to your changes of course. But it's be nice just to keep the two repositories completely in sync with one another.
Fish, Change made. I would prefer not to rebase my CU14 branch to pick up your two minor commits. I always mess up my git rebase attempts. Thanks for the quick review. Jim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I know I'm being anal (so sue me for having OCD!), but I've finished reviewing your two tests (CU14-01 and CU14-02), and while CU14-02 looks fine, CU14-01 has two typos (misspellings) in it.
Yes, I know simple typos/misspellings in comments are no big deal and don't impact the actual functioning/correctness of the program, BUT...
As I said, it's my OCD/anal-ness (is that a word?) kicking in. :)
None of the tests are through.
None of the tests are thorough.
cross pabe bounday
cross page bounday
It's ENTIRELY OPTIONAL whether you want to bother fixing them or not! If you don't want to bother, let me know and I'll approve your Pull Request as-is, because everything else looks fine.
So even though this code review is requesting changes, the changes being requested DO NOT have to be made! It's entirely up to you. I don't want my nit-picking to discourage you. IMHO the code (changes) you've contributed to Hercules are top-notch, and I VERY MUCH appreciate your efforts/contributions! I mean that sincerely, James!
So just let me know whether you want me to accept your Pull Request as-is or not, or whether you want to fix your typos.
Your choice. I don't care one way or the other.
That's fine. As I said, no biggie. |
p.s. is there a reason your two tests are using Again, no big deal. |
SEMI-RELATED QUESTION: What kind of processor does your system have? My system's processors are 2.93GHz X5570 Intel Xeons (it's an older system), and the speeds I'm getting are: Before:
After:
Which is indeed an 83% performance improvement (or about 6.25 times faster! WELL DONE, James!), but notice how much slower my times are compared to yours! Your system is a good 3 times faster than mine!! So I'm curious: What kind processors do you have and how fast are they running at?? |
Fish, I do appreciate all the review, including the comments. I usually have spelling errors in comments (as they are not tested!) and I don't read them after a while. I reduced the mainstor size to 4 for CU14-01-xpage and to 8 for CU14-02-performance (currently has a reference above the 6M boundary). I could reduce CU14-02 memory size but that would require a code change, and I'm a bit lazy. My performance numbers were from a 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz. Jim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate your capitulating to my OCD. :)
You didn't have to do that of course, but I DO appreciate it. That was very kind of you.
PR approved. Give me a second and I'll go ahead and do the merge.
Thanks again for everything!
Fish,
Here is a proposed performance improvement for the CU14 instruction.
Before the change, the performance test reported:
and after the change:
The performance increase ranges from 70 - 83%.
It has been a year plus since my last pull request, so I'm sure that I have missed something! I appreciate your review.
Once CU14 performance improvement is closed, I'll work on CU12 to finish Issue #101.
Jim