spec: add AliasReassignment by WalterBright · Pull Request #2919 · dlang/dlang.org

WalterBright · 2020-12-17T08:29:19Z

Implementation: dlang/dmd#11846

dlang-bot · 2020-12-17T08:29:20Z

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

ibuclaw · 2020-12-19T07:56:40Z

spec/declaration.dd

+        )
+
+        $(RATIONALE Alias reassignment can result in significantly faster compile times
+        and lowered memory consumption than the alternative recursive method.)


Citation? I was under the impression that this is currently inconclusive.

I don't need a test to know that:

malloc(5);

is faster and uses less memory than:

malloc(5); malloc(5);

:-)

dlang/phobos#7756 (comment) Some plots on this. The performance increase is measurable but not particularly dramatic for staticMap

I don't need a test to know that:

malloc(5);

is faster and uses less memory than:

malloc(5); malloc(5);

:-)

But that's exactly what static foreach is doing, it will syntaxCopy the foreach body N-times.
The rationale should simply be to provide an imperative alternative for constructing alias sequences vs the recursive way.
The performance is just an implementation detail. I hope it gives more performance though.

@WalterBright The consensus appears to be to revise this bit as:

$(RATIONALE Alias reassignment can result in faster compile times and lowered memory consumption,
and requires significantly simpler code than the alternative recursive method.)

12345swordy · 2020-12-30T21:29:37Z

@atilaneves there is a lot of spec pull requests that @WalterBright has made that needs review from you as you took over @andralex leadership position on this. Can you spare some time and review this PR and other PR likes this that Walter has made?

mdparker · 2021-03-15T11:05:51Z

@atilaneves The text looks good editorially. I'm good for merging this if you and @ibuclaw are good now with the objection you raised.

Imperatorn · 2021-05-04T18:12:08Z

Merge?

maxhaton · 2021-05-04T21:29:16Z

Merge?

Needs WalTila approval.

@WalterBright @atilaneves

It's been something like 7 months now, needs a decision.

There are probably lessons to be learned from how this has been handled.

I say just merge, if something bad happens due to Alias Assign down the line it will likely be due to the implementation, and if we decide it's bad language design then the design itself can be deprecated reasonably smoothly as its use should be both fairly limited and mostly by the hands of people who know what they're doing and are either maintainers or in direct contact with maintainers.

atilaneves · 2021-05-10T13:27:45Z

Does anyone have numbers on the actual impact on compile-times due to this?

maxhaton · 2021-05-10T14:42:16Z

Does anyone have numbers on the actual impact on compile-times due to this?

I have some nice graphs on some microbenchmarks, I spoke to John and Stefan about it IRL a few days ago, must have forgotten to post them. Will do so tomorrow (exam today) .

Rough summary is that AliasAssign seems like a win in all but the extreme (when it becomes really really bad, but it's almost definitely an implementation issue, but sufficiently weird dynamics that it's hard to work out from what)

mdparker · 2021-05-21T01:58:14Z

@maxhaton So what's the verdict? Is the performance claim good to go?

andralex · 2021-05-21T02:10:55Z

FWIW in all my real-world testing (building large apps) AliasAssignment has been a win or break even, with huge gains in code size and readability.

mdparker · 2021-05-21T02:16:33Z

Then I propose:

$(RATIONALE Alias reassignment can result in faster compile times and lowered memory consumption, 
and requires significantly fewer lines of code than the alternative recursive method.)

andralex · 2021-05-21T02:18:53Z

Then I propose:

$(RATIONALE Alias reassignment can result in faster compile times and lowered memory consumption, 
and requires significantly fewer lines of code than the alternative recursive method.)

s/fewer lines of/simpler/

mdparker · 2021-05-21T02:19:45Z

Paging @WalterBright.

mdparker · 2021-05-21T02:21:44Z

s/fewer lines of/simpler/

Then perhaps also s/requires/results in/.

maxhaton · 2021-05-21T04:07:55Z

FWIW in all my real-world testing (building large apps) AliasAssignment has been a win or break even, with huge gains in code size and readability.

I agree/concur with Andrei. This feature is still a little raw (Different syntax? It's a bit weird as is in the sense that it's not quite an assignment given the restrictions), but the code-size wins are really nice, and the blow-up losses seem to be an implementation problem which we can nail down (they are awful when you hit them, but we're talking staticMaps over thousands and thousands of elements here).

atilaneves · 2021-05-24T16:19:54Z

FWIW in all my real-world testing (building large apps) AliasAssignment has been a win or break even, with huge gains in code size and readability.

How big was the median win?

andralex · 2021-05-24T22:57:31Z

FWIW in all my real-world testing (building large apps) AliasAssignment has been a win or break even, with huge gains in code size and readability.

How big was the median win?

Say some 5%-7% but on a rather noisy setup that measured only the whole project bottom line. Let me know if precision is needed and I can set up a more rigorous experimental rig.

CyberShadow · 2021-06-26T01:32:23Z

Either the compiler behavior needs to be put behind a -preview flag (I'm guessing not doable because Phobos is using this) or this needs to be merged, because right now the reference implementation's behavior does not correspond to the official specification.

We can help prevent incidents like these from happening in the future by requiring (i.e. enforcing as part of CI) that the compiler test suite / Phobos / etc. are valid according to the official grammar. This will require first making a pull request to dlang.org, describing the language change, which only then will allow any changes to the reference implementation (DMD) to be merged.

mdparker · 2021-06-28T06:41:30Z

spec/declaration.dd

+        )
+
+        $(RATIONALE Alias reassignment can result in significantly faster compile times
+        and lowered memory consumption than the alternative recursive method.)


@WalterBright The consensus appears to be to revise this bit as:

$(RATIONALE Alias reassignment can result in faster compile times and lowered memory consumption,
and requires significantly simpler code than the alternative recursive method.)

Implemented suggested change.

CyberShadow · 2021-07-04T04:04:39Z

spec/declaration.dd

+        $(GNAME AliasReassignment):
+            $(GLINK_LEX Identifier) $(D =) $(GLINK StorageClasses)$(OPT) $(GLINK Type)
+            $(GLINK_LEX Identifier) $(D =) $(GLINK2 expression, FunctionLiteral)
+            $(GLINK_LEX Identifier) $(D =) $(GLINK StorageClasses)$(OPT) $(GLINK BasicType) $(GLINK2 function, Parameters) $(GLINK2 function, MemberFunctionAttributes)$(OPT)


Sorry, how is this different from the AliasAssign rule added in #2868?

Furthermore, AliasReassignment is not referenced anywhere in the spec, which effectively makes this syntax not a part of the language. What does this grammar block describe exactly?

@WalterBright

@WalterBright ping

CyberShadow · 2021-07-04T04:49:34Z

spec/declaration.dd

+
+        $(GRAMMAR
+        $(GNAME AliasReassignment):
+            $(GLINK_LEX Identifier) $(D =) $(GLINK StorageClasses)$(OPT) $(GLINK Type)


Type is declared in type.dd, not in this file.

CyberShadow · 2021-07-04T04:49:51Z

spec/declaration.dd

+        $(GNAME AliasReassignment):
+            $(GLINK_LEX Identifier) $(D =) $(GLINK StorageClasses)$(OPT) $(GLINK Type)
+            $(GLINK_LEX Identifier) $(D =) $(GLINK2 expression, FunctionLiteral)
+            $(GLINK_LEX Identifier) $(D =) $(GLINK StorageClasses)$(OPT) $(GLINK BasicType) $(GLINK2 function, Parameters) $(GLINK2 function, MemberFunctionAttributes)$(OPT)


Ditto for BasicType.

WalterBright added the Spec / content label Dec 17, 2020

WalterBright requested a review from andralex as a code owner December 17, 2020 08:29

WalterBright mentioned this pull request Dec 17, 2020

extend PR #11846 to add symbol support for AliasAssign dlang/dmd#12029

Merged

WalterBright force-pushed the AliasReassignment branch from e4780dd to d0fd9ab Compare December 17, 2020 08:44

ibuclaw reviewed Dec 19, 2020

View reviewed changes

WalterBright force-pushed the AliasReassignment branch from d0fd9ab to 99a7dbb Compare March 4, 2021 08:14

mdparker mentioned this pull request Mar 15, 2021

add new feature AliasAssign to spec #2868

Merged

CyberShadow mentioned this pull request Jun 26, 2021

Ensure all DMD test/compilable tests parse successfully CyberShadow/tree-sitter-d#2

Closed

mdparker previously requested changes Jun 28, 2021

View reviewed changes

spec: add AliasReassignment

bdd2d2b

WalterBright force-pushed the AliasReassignment branch from 99a7dbb to bdd2d2b Compare July 3, 2021 06:11

mdparker approved these changes Jul 3, 2021

View reviewed changes

mdparker merged commit 969ceb1 into dlang:master Jul 3, 2021

CyberShadow reviewed Jul 4, 2021

View reviewed changes

CyberShadow mentioned this pull request Jul 4, 2021

spec/declaration.dd: Update grammar syntax in AliasAssign / AliasReassignment #3054

Merged

Uh oh!

Conversation

WalterBright commented Dec 17, 2020

Uh oh!

dlang-bot commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

12345swordy commented Dec 30, 2020

Uh oh!

mdparker commented Mar 15, 2021

Uh oh!

Imperatorn commented May 4, 2021

Uh oh!

maxhaton commented May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atilaneves commented May 10, 2021

Uh oh!

maxhaton commented May 10, 2021

Uh oh!

mdparker commented May 21, 2021

Uh oh!

andralex commented May 21, 2021

Uh oh!

mdparker commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andralex commented May 21, 2021

Uh oh!

mdparker commented May 21, 2021

Uh oh!

mdparker commented May 21, 2021

Uh oh!

maxhaton commented May 21, 2021

Uh oh!

atilaneves commented May 24, 2021

Uh oh!

andralex commented May 24, 2021

Uh oh!

CyberShadow commented Jun 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Comments

dlang-bot commented Dec 17, 2020 •

edited

Loading

maxhaton commented May 4, 2021 •

edited

Loading

mdparker commented May 21, 2021 •

edited

Loading