Stop eagerly generating error message for mismatch arguments by Geod24 · Pull Request #8216 · dlang/dmd

Geod24 · 2018-05-04T15:56:48Z

This mostly reverts commit 15b1b503440caab6ba8d2879b80d3946d4d5ac23.

The aforementioned commit eagerly allocated the error message during call resolution,
resulting in a lot of garbage being generated for no reason.
Said garbage was never freed, and freeing it seemingly has no effect,
maybe because of DMD's memory allocation pattern.

Testing here since the testsuite doesn't pass on my machine ¯_(ツ)_/¯
Also, targeting stable because I expect large project to suffer from this a lot.

dlang-bot · 2018-05-04T15:56:49Z

Thanks for your pull request and interest in making D better, @Geod24! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + dmd#8216"

wilzbach · 2018-05-04T16:03:12Z

Testing here since the testsuite doesn't pass on my machine ¯_(ツ)_/¯

Interesting. What error do you get?
Maybe the stack traces without line numbers? (e.g. dlang/druntime#2172)

This mostly reverts commit 15b1b50. The aforementioned commit eagerly allocated the error message during call resolution, resulting in a lot of garbage being generated for no reason. Said garbage was never freed, and freeing it seemingly has no effect, maybe because of DMD's memory allocation pattern.

Geod24 · 2018-05-05T13:07:12Z

It's green :)

ntrel · 2018-05-06T14:29:49Z

This pull degrades error messages significantly. What memory usage do you get with my fix? #7554 (comment)

If my fix is not sufficient, the proper solution would be to usefree and find out why dmd doesn't free memory properly (it could have an alternative strategy that works better withfree, enabled with a new compiler switch for intensive projects). This would greatly help a wider scope of projects besides yours.

Geod24 · 2018-05-07T00:59:42Z

This pull degrades error messages significantly.

But the test suite is passing. Which means the original pull request was significantly lacking tests. I haven't had a chance to test the fix you proposed yet, but I'm definitely not found of this approach.

JinShil · 2018-05-07T01:23:52Z

@ntrel Can you please submit a PR with your suggestion and sufficient coverage?

wilzbach

Let's get this into stable as the performance regressions seems is critical.
@ntrel please feel free to submit a follow-up with more test cases. I will open a PR to merge this to master too.

wilzbach · 2018-05-08T00:50:48Z

src/dmd/func.d

+
+        // disambiguate when toChars() is the same
+        auto at = arg.type.toChars();
+        bool qual = !arg.type.equals(par.type) && strcmp(at, par.type.toChars()) == 0;


"strcmp must die."
(I am aware that this is moved existing code)

Not sure what you're quoting. What should it be replaced with?

Not sure what you're quoting.

Walter from DConf and the mailing list.

What should it be replaced with?

== (with D's strings), though that might be a bit difficult to achieve here as still many places lack the ability to use d strings.

wilzbach · 2018-05-08T01:14:45Z

(update the C++ headers)

ntrel · 2018-05-08T16:58:13Z

@wilzbach This pull mucks up my pull here (which I forgot hadn't been merged). That pull is completely incompatible with just returning a parameter index.

ntrel · 2018-05-08T17:09:08Z

@JinShil I've now submitted #8232. If I have time I might try to improve the test suite.

@Geod24:

I haven't had a chance to test the fix you proposed yet, but I'm definitely not found of this approach

My pull fixes the only case where an error could be allocated and not used. Returning an error string is necessary, otherwise we would have to combine and return various other data besides just a parameter index.

ntrel · 2018-05-10T11:48:02Z

src/dmd/func.d

+    if (failIndex < fargs.dim && failIndex < tf.parameters.dim)
+    {
+        auto arg = (*fargs)[failIndex];
+        auto par = (*tf.parameters)[failIndex];


failIndex is the argument index, not parameter, so for typesafe variadics with more than one argument, no supplemental error will be shown. (This could be worked around perhaps). I'll send a PR to test this.

ntrel · 2018-05-10T11:55:51Z

This pull degrades error messages significantly

The only problem I've found is with typesafe variadics - PR for a test against this branch: Geod24#2

ntrel · 2018-05-19T15:06:32Z

BTW #8232 has now been merged. I've also made #8263 (against master for now) which removes 2 temporary allocations and adds the missing typesafe variadic test I mentioned.

RazvanN7 · 2018-05-24T10:58:01Z

Now that @ntrel 's PRs have been merged. Is this needed anymore?

Geod24 · 2018-05-28T07:55:44Z

Now that @ntrel 's PRs have been merged. Is this needed anymore?

Nope

Geod24 requested a review from wilzbach May 4, 2018 15:56

Geod24 force-pushed the partially-revert-15613 branch 2 times, most recently from bbc7ae3 to 27b747c Compare May 5, 2018 10:12

Geod24 force-pushed the partially-revert-15613 branch from 27b747c to a44c4fa Compare May 5, 2018 10:41

Geod24 mentioned this pull request May 6, 2018

Fix Issue 15613, 11529: Show parameter mismatch and rvalue/lvalue ref message #7554

Merged

wilzbach approved these changes May 8, 2018

View reviewed changes

Update C++ headers for failedIndex

7fc2e43

Geod24 requested review from dnadlinger and ibuclaw as code owners May 8, 2018 01:11

wilzbach added the Review:stable-priority A regression fix to stable that should receive higher priority label May 8, 2018

ntrel mentioned this pull request May 8, 2018

Avoid eager error message allocation with functionResolve #8232

Merged

ntrel reviewed May 10, 2018

View reviewed changes

Geod24 closed this May 28, 2018

Geod24 deleted the partially-revert-15613 branch June 26, 2018 09:14

Uh oh!

Comments

Conversation

Geod24 commented May 4, 2018

Uh oh!

dlang-bot commented May 4, 2018

Bugzilla references

Testing this PR locally

Uh oh!

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

Uh oh!

Uh oh!

Geod24 commented May 5, 2018

Uh oh!

ntrel commented May 6, 2018

Uh oh!

Geod24 commented May 7, 2018

Uh oh!

JinShil commented May 7, 2018

Uh oh!

wilzbach left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach May 8, 2018

Choose a reason for hiding this comment

Uh oh!

ntrel May 8, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach May 8, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach commented May 8, 2018

Uh oh!

ntrel commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ntrel commented May 8, 2018

Uh oh!

ntrel May 10, 2018

Choose a reason for hiding this comment

Uh oh!

ntrel commented May 10, 2018

Uh oh!

ntrel commented May 19, 2018

Uh oh!

RazvanN7 commented May 24, 2018

Uh oh!

Geod24 commented May 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wilzbach commented May 4, 2018 •

edited

Loading

wilzbach left a comment •

edited

Loading

ntrel commented May 8, 2018 •

edited

Loading