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

Deprecate Uuid#setFactory(), use dedicated factories for FieldInterface and for UUID creation #326

Open
Ocramius opened this issue Jul 7, 2020 · 2 comments

Comments

@Ocramius
Copy link
Sponsor Contributor

Ocramius commented Jul 7, 2020

Problem-space

While working on #324, it became evident that the majority of complexity within this library comes from Uuid#setFactory(), which completely switches the behavior of the (supposedly) @psalm-pure operations, as well as the various Uuid::uuid1() through Uuid::uuid6() methods.

Solution

I suggest deprecating and discouraging usage of Uuid#setFactory() and Uuid#getFactory(), as well as deprecating all setters on UuidFactory itself, so that post-instantiation no internals of it can change.

After that, we can drop all this API for 5.0.0, allowing for major performance and design improvements within the core of the library.

Further scope

  • The UuidFactoryInterface also needs to be split up a bit: there is no need for instantiating specific UUID sub-types unless the user relies on them, so I'd suggest having dedicated API for that, and dedicated API for retrieving FieldsInterface.
  • The FeatureSet is also extremely bloated: while flexible, it leads to a lot of added complexity for supporting things like GUIDs, while separate independent UuidFactoryInterface concrete implementations would be better for both clarity and runtime performance.
  • Aggressive inlining can be applied to factory internals once we have split up all details: could even be interesting to use FFI for such purposes, but that is yet to be verified by benchmarks.
@ramsey
Copy link
Owner

ramsey commented Jul 8, 2020

After reading through #324, it became clear to me why setFactory() was so problematic, so I agree with deprecating it and getFactory(), as well as the setters on UuidFactory.

The FeatureSet is also extremely bloated

I agree, and I'd like to throw it out. This was originally a way to swap out components for testing, and it has grown significantly since introduced. Independent concrete implementations of UuidFactoryInterface sounds like the way to go.

Aggressive inlining can be applied to factory internals

I'd like to hear more about this. I'm not familiar with the term "inlining."

@Ocramius
Copy link
Sponsor Contributor Author

Ocramius commented Jul 9, 2020

I'd like to hear more about this. I'm not familiar with the term "inlining."

Essentially copy-pasting static code to reduce the overhead from stack frame allocation. In static languages, the compiler does that implicitly, as an optimization.

While dependency injection does ease testing in this library, most of it is static code, if you assume only defaults.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants