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

Faster PHP solution. #991

Open
wants to merge 4 commits into
base: drag-race
Choose a base branch
from

Conversation

andrewthecodertx
Copy link

@andrewthecodertx andrewthecodertx commented Dec 21, 2024

Description

PHP solution that is about 5x faster than solution1

Contributing requirements

  • I read the contribution guidelines in CONTRIBUTING.md.
  • I placed my solution in the correct solution folder.
  • I added a README.md with the right badge(s).
  • I added a Dockerfile that builds and runs my solution.
  • I selected drag-race as the target branch.
  • All code herein is licensed compatible with BSD-3.

@rbergen
Copy link
Contributor

rbergen commented Dec 21, 2024

@andrewthecodertx Thank you for your submission. I've taken a look at it, and there are some things that I think need to be addressed before I can consider this PR further:

  • This is the main one: the results of the solution are invalid, because the sieve size is wrong. You set the sieve size to 100,000. Because of that it yields a prime count of 9,592, which is not the correct value for a sieve size of 1,000,000. In fact, the solution's own output clearly states it isn't:

    Passes: 6592, Time: 5000ms, Avg: 0ms, Limit: 1000000, Count: 9592, Valid: False
    
    andrewthecoder;6592;5.000142;1;algorithm=base,faithful=yes,bits=8
    
  • The rules for a faithful solution indicate that:

    The sieve size and corresponding prime candidate memory buffer (or language equivalent) are set/allocated dynamically at runtime. The size of the memory buffer must correspond to the size of the sieve.

    In your code, the sieve size is effectively fixed to 100,000 because the sieve class constructor does not accept a parameter to set it. That would make this solution unfaithful.

  • This solution has the same basic characteristics (algorithm, faithfulness, number of bits) as solution_1. As per the contribution guidelines, that usually means that if a speed improvement can be implemented, a PR to improve the existing solution should be opened instead of proposing to create a new solution. In this particular case I've compared the code of solution_1 to the solution you propose to add, and they are really very similar. Therefore, I would certainly not accept this as a new solution.

If you correct the first two issues and then still conclude there is a meaningful speed improvement compared to solution_1, then I suggest you modify this PR to augment solution_1 with the actual algorithmic differences that make that speed improvement happen (thereby addressing the third point).

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