Skip to content
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

Merged
merged 3 commits into from
Feb 12, 2024
Merged

Conversation

JamesWekel
Copy link
Contributor

Fish,

Here is a proposed performance improvement for the CU14 instruction.

Before the change, the performance test reported:

        1,000,000 iterations of CU14  took     846,440 microseconds
        1,000,000 iterations of CU14  took     879,949 microseconds
        1,000,000 iterations of CU14  took     855,963 microseconds
        1,000,000 iterations of CU14  took     855,836 microseconds
        1,000,000 iterations of CU14  took  73,516,592 microseconds

and after the change:

        1,000,000 iterations of CU14  took     172,939 microseconds
        1,000,000 iterations of CU14  took     200,985 microseconds
        1,000,000 iterations of CU14  took     204,565 microseconds
        1,000,000 iterations of CU14  took     223,280 microseconds
        1,000,000 iterations of CU14  took  10,825,924 microseconds

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

Copy link
Member

@Fish-Git Fish-Git left a 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.

@JamesWekel
Copy link
Contributor Author

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

Copy link
Member

@Fish-Git Fish-Git left a 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.

@Fish-Git
Copy link
Member

I would prefer not to rebase my CU14 branch to pick up your two minor commits. I always mess up my git rebase attempts.

That's fine. As I said, no biggie.

@Fish-Git
Copy link
Member

p.s. is there a reason your two tests are using mainstor 16? Isn't that overkill? Looking at your tests, it seems to me that 4MB would probably be plenty!

Again, no big deal.

@Fish-Git
Copy link
Member

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:

    1,000,000 iterations of CU14  took   2,588,625 microseconds
    1,000,000 iterations of CU14  took   2,352,916 microseconds
    1,000,000 iterations of CU14  took   2,352,945 microseconds
    1,000,000 iterations of CU14  took   2,413,947 microseconds
    1,000,000 iterations of CU14  took 195,244,598 microseconds

After:

    1,000,000 iterations of CU14  took     439,171 microseconds
    1,000,000 iterations of CU14  took     511,245 microseconds
    1,000,000 iterations of CU14  took     515,241 microseconds
    1,000,000 iterations of CU14  took     624,051 microseconds
    1,000,000 iterations of CU14  took  32,890,569 microseconds

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??

@JamesWekel
Copy link
Contributor Author

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

Copy link
Member

@Fish-Git Fish-Git left a 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-Git Fish-Git merged commit 1eeb70a into SDL-Hercules-390:develop Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants