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

Feature - LIFO Execution Order (Matching Go's Behavior) #3

Merged
merged 3 commits into from
Oct 21, 2019

Conversation

Rican7
Copy link
Contributor

@Rican7 Rican7 commented Oct 10, 2019

Scope

This PR is a re-implementation of #2, with the differences being that:

  • It changes the default implementation (rather than providing an alternative) to match Go's behavior of LIFO execution order.
  • It uses the built-in SplStack data-structure for the internal stacking context implementation.

Credit

Credit goes to @francislavoie for this idea, in which this implementation was based off-of! He even encouraged me to contribute via my own PR (#2 (comment)). Thanks!

`defer` behavior of "stacking" and executing in LIFO order

Also switching the context to use the `SplStack` built-in data-structure
to provide the LIFO ordering and to simplify the implementation.

(Credit to @francislavoie for this idea, in which this implementation
was based off-of! Thanks!)

(See php-defer#2)
@francislavoie
Copy link

I think it would be better to reorder the defer calls in the test rather than changing the assertion. Reads weird to see 132 instead of 123 😅

@Rican7
Copy link
Contributor Author

Rican7 commented Oct 10, 2019

I think it would be better to reorder the defer calls in the test rather than changing the assertion. Reads weird to see 132 instead of 123 😅

Fair point! Will do!

the expected behavior without having an awkward out-of-order sentence
@bkrukowski
Copy link
Member

@Rican7 @francislavoie

Really nice implementation, but I have mixed feelings. Do we really need the same behavior as in GO? Honestly when I prepared this library I didn't think about use cases when user will really need more than 1 defer callback. If we want to have exactly the same behavior as in GO, I'd like to release 2 more major versions. Version 3.0 for PHP ^5.3 and ^7.0 and version 4.0 for PHP ^7.1. Please tell me what do you think. Please share use cases, even the most weird.

Second think - based on this library it would be super easy to prepare simple profiler for PHP.

function myFunction() {
    // we don't call start and stop, it's a magic :)
    // even it's not needed to pass __FUNCTION__ or __METHOD__
    DeferProfiler::measure($_);

    // logic here
}

In output we would be able to print how many times given function was executed, average and the highest execution time. Do you have other ideas? Would you like to contribute to such project?

@Rican7
Copy link
Contributor Author

Rican7 commented Oct 11, 2019

@bkrukowski

There's a bit to unpack here.

Really nice implementation, but I have mixed feelings. Do we really need the same behavior as in GO? Honestly when I prepared this library I didn't think about use cases when user will really need more than 1 defer callback.

I understand the hesitance here, but yea there's a reason why Go chose LIFO order for defers. In fact, @NoiseByNorthwest did a good job succinctly demonstrating the reasoning in their comment here: #2 (comment).

Essentially, defer (especially multiple) is mostly useful for resource management, such as closing files, streams, and other contexts. Since "normal" code is executed top-to-bottom (within a scope), a resource can be passed or referred to later on in a scope, so defer execution needs to be done in the opposite order of creation to guarantee that there's nothing "dangling".

You may personally not run into this often, which is maybe why you mentioned that you "didn't think about use cases when user will really need more than 1 defer callback", and that's fine, but its a real problem.

There's more on Go's defer in the "Effective Go" document.

If we want to have exactly the same behavior as in GO, I'd like to release 2 more major versions. Version 3.0 for PHP ^5.3 and ^7.0 and version 4.0 for PHP ^7.1. Please tell me what do you think. Please share use cases, even the most weird.

I understand wanting to maximize compatibility, but I implore you to please re-think this. PHP 5.3 is over 10 years old, is no longer supported, and has multiple security vulnerabilities.

In fact, PHP 7.1 is only receiving security patches right now, and that support ends in a little over a month:

In fact, WordPress isn't even supporting it anymore (and they're incredibly conservative and are usually the last to update their requirements): https://wordpress.org/news/2019/04/minimum-php-version-update/

I'd strongly suggest keeping your current supported versions as-is.

Second think - based on this library it would be super easy to prepare simple profiler for PHP.

function myFunction() {
    // we don't call start and stop, it's a magic :)
    // even it's not needed to pass __FUNCTION__ or __METHOD__
    DeferProfiler::measure($_);

    // logic here
}

In output we would be able to print how many times given function was executed, average and the highest execution time. Do you have other ideas? Would you like to contribute to such project?

That's a clever idea! I see what you mean, as the measure method could start profiling the moment its called, and then stop at "destruction" time, based on the past context.

I personally don't use PHP much anymore, at least not professionally, so I wouldn't be the right person to maintain such a project, but I don't think its a bad idea. 🙂

@bkrukowski
Copy link
Member

I understand the hesitance here, but yea there's a reason why Go chose LIFO order for defers.

Now I can see more advantages, e.g.

function myFn() {
    DeferProfiler::measure($_);
    defer($_, function () {
        // deferred code
    });
    // logic
}

So I'm almost convinced. Give me a while to think and read about it.

I understand wanting to maximize compatibility, but I implore you to please re-think this. PHP 5.3 is over 10 years old, is no longer supported, and has multiple security vulnerabilities. (...)

It would take only few minutes to prepare code which supports older versions. Official support of given version is one thing, real usage is unfortunately second thing.

https://blog.packagist.com/php-versions-stats-2019-1-edition/

@Rican7
Copy link
Contributor Author

Rican7 commented Oct 11, 2019

I understand the hesitance here, but yea there's a reason why Go chose LIFO order for defers.

Now I can see more advantages, e.g.

function myFn() {
    DeferProfiler::measure($_);
    defer($_, function () {
        // deferred code
    });
    // logic
}

So I'm almost convinced. Give me a while to think and read about it.

Awesome! Thank you! 😀

I understand wanting to maximize compatibility, but I implore you to please re-think this. PHP 5.3 is over 10 years old, is no longer supported, and has multiple security vulnerabilities. (...)

It would take only few minutes to prepare code which supports older versions. Official support of given version is one thing, real usage is unfortunately second thing.

https://blog.packagist.com/php-versions-stats-2019-1-edition/

That link could maybe support an argument for supporting 5.6 and up, but you'd still be supporting an EOL version of the language, and that might hurt later more than it'd help. But that's up to you.

@francislavoie
Copy link

francislavoie commented Oct 11, 2019

It would take only few minutes to prepare code which supports older versions. Official support of given version is one thing, real usage is unfortunately second thing.

I would say that could be a valid reason for an existing/old library, but I don't think it's a good argument for a new library. Any new code should be targeting at least the oldest still-supported version. I really doubt anyone still running older versions will suddenly decide they need to use this lib in their older projects. If they really need to, they can always fork it.

@bkrukowski bkrukowski mentioned this pull request Oct 21, 2019
@bkrukowski bkrukowski merged commit 9364da0 into php-defer:master Oct 21, 2019
@Rican7 Rican7 deleted the feature/lifo-execution-order branch October 21, 2019 15:48
@Rican7
Copy link
Contributor Author

Rican7 commented Oct 21, 2019

Thanks for the merge @bkrukowski!

bkrukowski added a commit that referenced this pull request Oct 21, 2019
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.

None yet

3 participants