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

implemented Sieve of Eratosthenes in cpp #233

Closed
wants to merge 0 commits into from

Conversation

dibyam-jalan27
Copy link

Description

Type of Change

  • ✨ New snippet
  • πŸ›  Improvement to an existing snippet
  • 🐞 Bug fix
  • πŸ“– Documentation update
  • πŸ”§ Other (please describe):

Checklist

  • I have tested my code and verified it works as expected.
  • My code follows the style and contribution guidelines of this project.
  • Comments are added where necessary for clarity.
  • Documentation has been updated (if applicable).
  • There are no new warnings or errors from my changes.

Related Issues

Closes #

Additional Context

Screenshots (Optional)

Click to view screenshots

Copy link
Collaborator

@Mathys-Gasnier Mathys-Gasnier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it looks good, let's wait on another reason before merging

Copy link
Collaborator

@majvax majvax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, Thanks for you contribution ! πŸ™Œ

However, I have few question/suggestion that I'm free to discuss to make this snippet better.

Instead of returning a std::vector<int> why don't you return std::vector<bool>.
It make it more efficient to find if a certain value is prime or not (O(1) for vec<bool> and O(log n) for vec<int>, using binary search.).

An int take up to 8 bytes in memory as a bool take only one. For primes up to n we can wrote this formula:

  • vec<bool> = ~n
  • vec<int> = ~(n/ln(n)) * sizeof(int)

For n = 100_000, vec<bool> uses 12500 bytes as vec<int> uses 38000 bytes. (used int=4bytes)

Consider adding const to your parameter and noexcept to your function.
Also, if you are still planning to use a vec<int>, consider reserving the memory using the prime number theorem to make less allocation.

  • primes.reserve(n/std::log(n));
  • or, for a more precise allocation:
    double ln_n = std::log(n);
    double estimate = (n / ln_n) * (1 + 1/ln_n + 1.8/(ln_n*ln_n));
    primes.reserve(estimate*1.005);
    
  • Or, a for a bit precision allocation:
    size_t count = 0;
    for (int i = 2; i <= n; ++i) {
        if (is_prime[i]) count++;
    }   
    std::vector<int> primes;
    primes.reserve(count);
    

Letting the vector grow itself seems a bad idea, when benchmarking, for n = 1000000 I got:

For n = 1000000:
there should be 78695.8 primes
Number of primes: 78498
Vector<int> size in bytes: 313992
Vector<int> capacity in bytes: 524288
Unused capacity in bytes: 210296

Even for smaller n, I always got 30-70% byte loss and for bigger n, the problem is worse

@dibyam-jalan27
Copy link
Author

I am using windows and unable to commit due to prettier rules. How can i fix that?

@majvax
Copy link
Collaborator

majvax commented Jan 13, 2025

Can you please run

  • npm install
  • npm run snippets:check

If you are sure your commit is well formatted you can skip GitHub hook

It is strange that it doesn't allow you to commit since pre-commit hook doesn't contain any prettier related formatting.

Lastly I can only recommend to hard reset branch and open a new PR

@majvax
Copy link
Collaborator

majvax commented Jan 13, 2025

We won't help you if you close your PR definitely. Please open it again if you want to seek support.

@Mathys-Gasnier
Copy link
Collaborator

(Eslint runs prettier so it's probably why it fails, but I have no clue why it fails above that)

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.

3 participants