Use `atomic.Pointer` type to remove unsafe package usage directly

[OmidHekayati]

• Use atomic.Int64 in atomicInt64Limiter type to prevent any mistake non atomic usage
• Move config and option to separate files from ratelimit.go Go is a package base language, So it is better to have many small single responsible files instead a huge multi responsible file.
• moving config building to the implementation independent code
• Fixed some typos

[CLAassistant]

All committers have signed the CLA.

[rabbbit]

Hi,

Thanks for the contribution @OmidHekayati.

The PR seems to contain a bunch of unrelated changes:

• some style changes (that I'm not convinced about)
• creating config.go, which seems okay
• creating options.go, which I don't like - I liked that all the "public" interface was stored in ratelimit.go
• two changes to Int64 and atomicInt64Limiter, which seem okay, but also not like a major improvement - the package is small enough that we're very unlikely to use the types wrong accidentally. I think they have no impact on the performance, but also, I'm not sure how the padding works exactly. It might be worth doing some benchmarks.

Without looking to the merit of each change, in general, we prefer the changes to be self-contained. Would you be interested in creating new, smaller PRs?

[storozhukBM]

Agree with @rabbbit

[OmidHekayati]

The PR seems to contain a bunch of unrelated changes: Without looking to the merit of each change, in general, we prefer the changes to be self-contained. Would you be interested in creating new, smaller PRs?

Git has many features, You can rebase the PR to move all 3 commits separately to the main branch for any feature needs (almost usually for revert).

• some style changes (that I'm not convinced about)

Which style? use var instead of := or use init methods to correctly encapsulate data fields with its logic? new... or New... functions must use just to allocate(It is so simple in go), initialize and return a reference object, It is bad practice to move logic from object encapsulation to a function.

• creating config.go, which seems okay

Thanks ;)

• creating options.go, which I don't like - I liked that all the "public" interface was stored in ratelimit.go

Why do you like to put many things in one large file? I see many devs use this package without any options!

• two changes to Int64 and atomicInt64Limiter, which seem okay, but also not like a major improvement - the package is small enough that we're very unlikely to use the types wrong accidentally. I think they have no impact on the performance,

Using atomic types is not about improving performance, It is about improving readability.

but also, I'm not sure how the padding works exactly. It might be worth doing some benchmarks.

I didn't add this padding and I also don't know why you add it in the past.

[rabbbit]

Sorry for the delay in responding, this slipped my mind.

I didn't add this padding and I also don't know why you add it in the past.

Generally, I recommend reading on Chesterton's fence, it's a pretty cool concept. In this case though, if you look at https://cs.opensource.google/go/go/+/refs/tags/go1.20.6:src/sync/atomic/type.go;l=93 you'll see that these atomics are already aligned, which might (I'm not sure) conflict with our own padding - or at least make it unnecessary. Which brings us to:

Using atomic types is not about improving performance, It is about improving readability.

Your change has the potential of affecting performance negatively. This is something we try to avoid.

Which style? use var instead of := or use init methods to correctly encapsulate data fields with its logic? new... or New... functions must use just to allocate(It is so simple in go), initialize and return a reference object, It is bad practice to move logic from object encapsulation to a function.

These all sound subjective. We generally try to follow https://github.com/uber-go/guide/blob/master/style.md. If you find any style inconsistent with this guide, we'll be happy to apply it.

Git has many features, You can rebase the PR to move all 3 commits

We will not be merging this PR as is. We're happy to merge individual changes, where each is reviewed independently.

For now, closing this PR as "won't do".

[OmidHekayati]

Sorry for the delay in responding, this slipped my mind.

I didn't add this padding and I also don't know why you add it in the past.

Generally, I recommend reading on Chesterton's fence, it's a pretty cool concept. In this case though, if you look at https://cs.opensource.google/go/go/+/refs/tags/go1.20.6:src/sync/atomic/type.go;l=93 you'll see that these atomics are already aligned, which might (I'm not sure) conflict with our own padding - or at least make it unnecessary. Which brings us to:

I know about padding to align memory access, But it is about to add at most 7 bytes, not 56 bytes! And your comments tell something else about this padding!

         //lint:ignore U1000 Padding is unused but it is crucial to maintain performance
    // of this rate limiter in case of collocation with other frequently accessed memory.
    padding [56]byte // cache line size - state pointer size = 64 - 8; created to avoid false sharing.

Using atomic types is not about improving performance, It is about improving readability.

Your change has the potential of affecting performance negatively. This is something we try to avoid.

Why??

Which style? use var instead of := or use init methods to correctly encapsulate data fields with its logic? new... or New... functions must use just to allocate(It is so simple in go), initialize and return a reference object, It is bad practice to move logic from object encapsulation to a function.

These all sound subjective. We generally try to follow https://github.com/uber-go/guide/blob/master/style.md. If you find any style inconsistent with this guide, we'll be happy to apply it.

I had read uber Go style, But I can't find any style inconsistent in my PR!

Git has many features, You can rebase the PR to move all 3 commits

We will not be merging this PR as is. We're happy to merge individual changes, where each is reviewed independently.

For now, closing this PR as "won't do".

Ok, anyone inside your team that knows the reviewers can see my PR and made your desired changes, and send other PR.

Have a nice time