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

[REG2.066] Issue 14886 - std.parallelism.parallel with large static array seems to hang compile #5085

Merged
merged 3 commits into from Sep 22, 2015

Conversation

Projects
None yet
5 participants
@9rnsr
Member

9rnsr commented Sep 17, 2015

@yebblies

This comment has been minimized.

Show comment
Hide comment
@yebblies

yebblies Sep 17, 2015

Member

This PR is over 60% code style changes. This is the kind of formatting churn that I'm concerned about. I understand the need for better source formatting, but we need the format agreed on and automated, otherwise it will never end. These changes really don't belong in a regression fix PR.

Member

yebblies commented Sep 17, 2015

This PR is over 60% code style changes. This is the kind of formatting churn that I'm concerned about. I understand the need for better source formatting, but we need the format agreed on and automated, otherwise it will never end. These changes really don't belong in a regression fix PR.

@9rnsr

This comment has been minimized.

Show comment
Hide comment
@9rnsr

9rnsr Sep 17, 2015

Member

It's code style regression which had been caused by magicport. I'm just fixing it.

Member

9rnsr commented Sep 17, 2015

It's code style regression which had been caused by magicport. I'm just fixing it.

@yebblies

This comment has been minimized.

Show comment
Hide comment
@yebblies

yebblies Sep 17, 2015

Member

Arbitrarily reformatting code is not 'fixing it'. It's just creating churn.

Member

yebblies commented Sep 17, 2015

Arbitrarily reformatting code is not 'fixing it'. It's just creating churn.

@9rnsr

This comment has been minimized.

Show comment
Hide comment
@9rnsr

9rnsr Sep 17, 2015

Member
  • In other PR, I was recommended by Walter to convert C style for to D style foreach.
  • Line spaces and wrap long expression are revived from CDMD code.
Member

9rnsr commented Sep 17, 2015

  • In other PR, I was recommended by Walter to convert C style for to D style foreach.
  • Line spaces and wrap long expression are revived from CDMD code.
@yebblies

This comment has been minimized.

Show comment
Hide comment
@yebblies

yebblies Sep 17, 2015

Member

And what do either of those have to do with Issue 14886?

Member

yebblies commented Sep 17, 2015

And what do either of those have to do with Issue 14886?

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Sep 17, 2015

Member

I agree that formatting changes belongs in separate PRs. Please do not combine them. Short PRs are much easier to review. The same goes for refactoring changes. Separate them, please.

Member

WalterBright commented Sep 17, 2015

I agree that formatting changes belongs in separate PRs. Please do not combine them. Short PRs are much easier to review. The same goes for refactoring changes. Separate them, please.

@9rnsr

This comment has been minimized.

Show comment
Hide comment
@9rnsr

9rnsr Sep 17, 2015

Member

I like to clean code at first, because it will clarify the problematic point.

The cleanup commit and bugfixes are actually separated. I think it won't block reviewing.

Member

9rnsr commented Sep 17, 2015

I like to clean code at first, because it will clarify the problematic point.

The cleanup commit and bugfixes are actually separated. I think it won't block reviewing.

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Sep 17, 2015

Member

This PR has 465 lines of change. Complex PRs are hard enough to review without lumping in extensive refactorings. Please make them separate PRs. We've discussed this before.

It's not just reviewing the individual commits. It's:

  1. The "take it or leave it" of all or nothing.
  2. If a bug is introduced, the git bisect leads to giant diffs as the culprit, again, making things harder.
  3. Explorer behaves miserably when github presents large diffs.
  4. I have only so many lines on my monitor. Big changes are a big pain in the ass to review.

And sometimes, there are simply too many "clean up" refactorings going on, making it impossible to compare source code from one release to the next. This makes it really really hard to continue understanding how the code works. Stability has a virtue all its own, when lots of people have to work with the code.

Member

WalterBright commented Sep 17, 2015

This PR has 465 lines of change. Complex PRs are hard enough to review without lumping in extensive refactorings. Please make them separate PRs. We've discussed this before.

It's not just reviewing the individual commits. It's:

  1. The "take it or leave it" of all or nothing.
  2. If a bug is introduced, the git bisect leads to giant diffs as the culprit, again, making things harder.
  3. Explorer behaves miserably when github presents large diffs.
  4. I have only so many lines on my monitor. Big changes are a big pain in the ass to review.

And sometimes, there are simply too many "clean up" refactorings going on, making it impossible to compare source code from one release to the next. This makes it really really hard to continue understanding how the code works. Stability has a virtue all its own, when lots of people have to work with the code.

@9rnsr

This comment has been minimized.

Show comment
Hide comment
@9rnsr

9rnsr Sep 18, 2015

Member

I removed the cleanup commit.

Member

9rnsr commented Sep 18, 2015

I removed the cleanup commit.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Sep 18, 2015

Member

We really should use dfmt at some point to avoid any formatting work an discussions.
I'd even be willing to live with the few issues dfmt still has.

Member

MartinNowak commented Sep 18, 2015

We really should use dfmt at some point to avoid any formatting work an discussions.
I'd even be willing to live with the few issues dfmt still has.

@yebblies

This comment has been minimized.

Show comment
Hide comment
@yebblies

yebblies Sep 18, 2015

Member

We really should use dfmt at some point to avoid any formatting work an discussions.

Agreed.

I'd even be willing to live with the few issues dfmt still has.

Mmm I'm not quite sure it's ready.

Member

yebblies commented Sep 18, 2015

We really should use dfmt at some point to avoid any formatting work an discussions.

Agreed.

I'd even be willing to live with the few issues dfmt still has.

Mmm I'm not quite sure it's ready.

@9rnsr

This comment has been minimized.

Show comment
Hide comment
@9rnsr

9rnsr Sep 18, 2015

Member

@MartinNowak magicport had removed many line spaces for logical code block separators. It won't be revived by dfmt.

Member

9rnsr commented Sep 18, 2015

@MartinNowak magicport had removed many line spaces for logical code block separators. It won't be revived by dfmt.

@9rnsr

This comment has been minimized.

Show comment
Hide comment
@9rnsr

9rnsr Sep 18, 2015

Member

@yebblies I removed all format changes in this PR. So, isn't this ready for review to you?

Member

9rnsr commented Sep 18, 2015

@yebblies I removed all format changes in this PR. So, isn't this ready for review to you?

@yebblies

This comment has been minimized.

Show comment
Hide comment
@yebblies

yebblies Sep 18, 2015

Member

Ok, so this avoids allocating thousands of elements in array literal default init, makes sense.

I'm concerned that this requires so many changes in so many places. I think it can be improved by providing foreach iteration over the expanded set of elements, instead of doing this manually in several places. Then most of the iterations would become foreach (e; &ale.expandElements).

Member

yebblies commented Sep 18, 2015

Ok, so this avoids allocating thousands of elements in array literal default init, makes sense.

I'm concerned that this requires so many changes in so many places. I think it can be improved by providing foreach iteration over the expanded set of elements, instead of doing this manually in several places. Then most of the iterations would become foreach (e; &ale.expandElements).

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Sep 18, 2015

Member

Your approach makes sense, but it's very invasive and brittle.
We should abstract the 2 iteration schemes, e.g. into a foreach construct or a simple "iterator" w/ a next() method. Also the name basis is somewhat unclear, how about defaultValue or so.

Member

MartinNowak commented Sep 18, 2015

Your approach makes sense, but it's very invasive and brittle.
We should abstract the 2 iteration schemes, e.g. into a foreach construct or a simple "iterator" w/ a next() method. Also the name basis is somewhat unclear, how about defaultValue or so.

for (size_t i = 0; i < e.elements.dim; i++)
{
Expression el = (*e.elements)[i];
if (result == MATCHnomatch)
break;
// no need to check for worse
if (!el)

This comment has been minimized.

@WalterBright

WalterBright Sep 20, 2015

Member

Why wasn't this check needed before?

@WalterBright

WalterBright Sep 20, 2015

Member

Why wasn't this check needed before?

This comment has been minimized.

@9rnsr

9rnsr Sep 20, 2015

Member

Because all elements in ArrayLiteralExp had been assumed to be non-null.

@9rnsr

9rnsr Sep 20, 2015

Member

Because all elements in ArrayLiteralExp had been assumed to be non-null.

This comment has been minimized.

@WalterBright

WalterBright Sep 21, 2015

Member

were they non-null?

@WalterBright

WalterBright Sep 21, 2015

Member

were they non-null?

This comment has been minimized.

@MartinNowak

MartinNowak Sep 21, 2015

Member

Yes, but this PR leaves them null and lazily fills them w/ basis thereby avoid repeated semantic calls on the same element.

@MartinNowak

MartinNowak Sep 21, 2015

Member

Yes, but this PR leaves them null and lazily fills them w/ basis thereby avoid repeated semantic calls on the same element.

Show outdated Hide outdated src/expression.d Outdated
@@ -4579,9 +4580,33 @@ public:
elements.push(e);
}
extern (D) this(Loc loc, Expression basis, Expressions* elements)

This comment has been minimized.

@WalterBright

WalterBright Sep 20, 2015

Member

When adding functions, please add Ddoc comment.

@WalterBright

WalterBright Sep 20, 2015

Member

When adding functions, please add Ddoc comment.

This comment has been minimized.

@9rnsr

9rnsr Sep 20, 2015

Member

It will become obvious by the document on basis field.

@9rnsr

9rnsr Sep 20, 2015

Member

It will become obvious by the document on basis field.

Show outdated Hide outdated src/constfold.d Outdated
@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Sep 20, 2015

Member

I removed the cleanup commit.

Thanks, Kenji!

Member

WalterBright commented Sep 20, 2015

I removed the cleanup commit.

Thanks, Kenji!

@9rnsr

This comment has been minimized.

Show comment
Hide comment
@9rnsr

9rnsr Sep 20, 2015

Member

Updated.

Member

9rnsr commented Sep 20, 2015

Updated.

Show outdated Hide outdated src/expression.d Outdated
@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Sep 21, 2015

Member

We should abstract the 2 iteration schemes, e.g. into a foreach construct or a simple "iterator" w/ a next() method.

Yeah, let's add 2 ranges for that, will make the code much clearer.
How about sparseElements and denseElements?

extern (C++) static struct SparseElements
{
    size_t idx;
    Expression* defaultValue;
    Expressions values;

    bool empty()
    {
        return !defaultValue && idx == values.dim;
    }

    Expression* front()
    {
        return (!idx && defaultValue) ? defaultValue : values[idx];
    }

    void popFront()
    {
        if (defaultValue)
            defaultValue = null;
        else
            ++idx;
    }
}

extern (C++) static struct DenseElements
{
    size_t idx;
    Expression* defaultValue;
    Expressions values;

    bool empty()
    {
        return idx == values.dim;
    }

    Expression* front()
    {
        return values[idx] ? values[idx] : defaultValue;
    }

    void popFront()
    {
        ++idx;
    }
}
Member

MartinNowak commented Sep 21, 2015

We should abstract the 2 iteration schemes, e.g. into a foreach construct or a simple "iterator" w/ a next() method.

Yeah, let's add 2 ranges for that, will make the code much clearer.
How about sparseElements and denseElements?

extern (C++) static struct SparseElements
{
    size_t idx;
    Expression* defaultValue;
    Expressions values;

    bool empty()
    {
        return !defaultValue && idx == values.dim;
    }

    Expression* front()
    {
        return (!idx && defaultValue) ? defaultValue : values[idx];
    }

    void popFront()
    {
        if (defaultValue)
            defaultValue = null;
        else
            ++idx;
    }
}

extern (C++) static struct DenseElements
{
    size_t idx;
    Expression* defaultValue;
    Expressions values;

    bool empty()
    {
        return idx == values.dim;
    }

    Expression* front()
    {
        return values[idx] ? values[idx] : defaultValue;
    }

    void popFront()
    {
        ++idx;
    }
}
@9rnsr

This comment has been minimized.

Show comment
Hide comment
@9rnsr

9rnsr Sep 22, 2015

Member

Rebased and resolved merge conflicts.

Member

9rnsr commented Sep 22, 2015

Rebased and resolved merge conflicts.

WalterBright added a commit that referenced this pull request Sep 22, 2015

Merge pull request #5085 from 9rnsr/fix14886
[REG2.066] Issue 14886 - std.parallelism.parallel with large static array seems to hang compile

@WalterBright WalterBright merged commit a492f91 into dlang:master Sep 22, 2015

3 checks passed

CyberShadow/DAutoTest Documentation OK (no changes)
Details
auto-tester Pass: 10
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Sep 22, 2015

Member

Thanks, Kenji!

Member

WalterBright commented Sep 22, 2015

Thanks, Kenji!

@9rnsr 9rnsr deleted the 9rnsr:fix14886 branch Sep 23, 2015

@schuetzm

This comment has been minimized.

Show comment
Hide comment
@schuetzm

schuetzm Sep 27, 2015

Contributor

Unfortunately, this introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=15123

Contributor

schuetzm commented Sep 27, 2015

Unfortunately, this introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=15123

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