Compute lazy lambda serialization by RazvanN7 · Pull Request #7870 · dlang/dmd

RazvanN7 · 2018-02-12T09:08:50Z

This is an optimization to compute the lambda serialization only when __traits(isSame) is used on lambda functions.

dlang-bot · 2018-02-12T09:08:51Z

Thanks for your pull request and interest in making D better, @RazvanN7! 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.

stefan-koch-sociomantic · 2018-02-12T13:34:48Z

+    override void visit(TypeClass t)
    {
-        buf.reset();
+        if (LOG)


stefan-koch-sociomantic · 2018-02-12T13:35:05Z

    override void visit(FuncLiteralDeclaration fld)
    {
        assert(fld.type.ty != Terror);
+        if (LOG)


andralex

cc @WalterBright

JinShil · 2018-02-12T18:39:24Z

There aren't any tests and coverage is only 80%. Is that deliberate?

RazvanN7 · 2018-02-13T09:31:59Z

@JinShil This PR does not introduce any new functionality, but optimizes the lambda comparison. The code is not covered because the struct and class cases are identical and I only added examples for one of them. I will add another few tests so that coverage is 100%.

P.S. : Everything is covered now.

timotheecour · 2018-02-13T10:08:37Z

@RazvanN7 speaking of __traits(isSame) and lambdas, there's a regression in git master:
https://issues.dlang.org/show_bug.cgi?id=18430 Issue 18430 - isSame is wrong for non global lambdas; obviously this PR couldn't have caused it since it's not merged but maybe you'd have some idea? (sorry for the drop in)

EDIT: just found out you already had a fix in progress #7885; thanks!

wilzbach · 2018-02-13T14:09:48Z

 import dmd.tokens;
 import dmd.visitor;

+enum LOG = false;


Out of interest: why not version= LOG? This is commonly used pattern in DRuntime.

This is the pattern used in dmd from what I could obeserve :

https://github.com/dlang/dmd/blob/master/src/dmd/mtype.d#L61
https://github.com/dlang/dmd/blob/master/src/dmd/expression.d#L67
https://github.com/dlang/dmd/blob/master/src/dmd/expressionsem.d#L73

And the list could continue.

wilzbach · 2018-02-13T14:11:46Z

-            import core.stdc.string;
+            const(char)* getSerialization(FuncLiteralDeclaration fld)
+            {
+                import dmd.lambdacomp;


Nit: local imports should be selective.

wilzbach · 2018-02-13T14:13:33Z

        if (l1 && l2)
        {
-            import core.stdc.string;
+            const(char)* getSerialization(FuncLiteralDeclaration fld)


Why don't you return string here? Comparing D strings is more efficient (no \0 checking)
You already know the length of the buffer - no need to throw this valuable information away.

I know it's easier with strings, but the serialization field is is const(char)* and I really haven't seen AST members declared as strings. All of them are const(char)*

Quoting a conversation with Walter here:

On 12/5/2017 11:20 AM, Andrei Alexandrescu wrote:
Hi Walter, Seb noticed that the dmd code is going through unpleasant hoops caused by C-style strings. Do you think incrementally switching to D-style strings would improve the compiler? -- Andrei

Yes, but we have to be mindful of gdc and ldc both relying on the C++ interface to use dmd, and D strings don't work on that interface.
I've already converted a bunch of internal stuff to D strings where the interface to the back end was not involved.

(we can even convert the C++ interfaces, but that's significantly more effort)

So how do we proceed here? That was my problem also: I may turn the field into a string, but what should I do regarding the header file?

Well, as its a new field and not used by GDC or LDC at the moment, you could do sth. like:

struct DString { size_t length; char* head; }

CC @ibuclaw @redstar @klickverbot

We have to start somewhere, but I don't want to block your PR with this. Sorry.

how about this:

extern(C++) Dstring funLib(Dstring input);

see full working example here showing using such API from C++ with the function defined in D.

https://github.com/timotheecour/dtools/blob/master/tests/d01_dmd_string/

EDIT: see #7893

@timotheecour #7893 is a good step in the right direction, we need to start using D strings in the compiler. That would be an effort separated from this.

RazvanN7 · 2018-02-14T14:23:22Z

I discovered some problems. Please do not merge.

Later edit: I solved the issue. Ready to merge.

andralex · 2018-02-20T18:13:10Z

        if (l1 && l2)
        {
-            import core.stdc.string;
+            const(char)* getSerialization(FuncLiteralDeclaration fld)


@timotheecour #7893 is a good step in the right direction, we need to start using D strings in the compiler. That would be an effort separated from this.

RazvanN7 force-pushed the Lambda_v2 branch 2 times, most recently from 93eee54 to 5fb1c17 Compare February 12, 2018 09:13

stefan-koch-sociomantic reviewed Feb 12, 2018

View reviewed changes

RazvanN7 force-pushed the Lambda_v2 branch from 5fb1c17 to a51aa6c Compare February 12, 2018 13:51

andralex approved these changes Feb 12, 2018

View reviewed changes

RazvanN7 force-pushed the Lambda_v2 branch from a51aa6c to 9390ce0 Compare February 13, 2018 09:39

wilzbach reviewed Feb 13, 2018

View reviewed changes

RazvanN7 force-pushed the Lambda_v2 branch 2 times, most recently from f788974 to d330e9e Compare February 14, 2018 09:43

RazvanN7 changed the title ~~Compute lazy lambda serialization~~ [WIP]Compute lazy lambda serialization Feb 14, 2018

timotheecour mentioned this pull request Feb 14, 2018

[WIP] add extern (C++) struct Darray(T) and Dstring to simplify interop with C++ #7893

Closed

RazvanN7 force-pushed the Lambda_v2 branch from d330e9e to 91c718a Compare February 15, 2018 12:08

RazvanN7 changed the title ~~[WIP]Compute lazy lambda serialization~~ Compute lazy lambda serialization Feb 15, 2018

Compute lazy lambda serialization

badb47e

RazvanN7 force-pushed the Lambda_v2 branch from 91c718a to badb47e Compare February 15, 2018 12:11

andralex approved these changes Feb 20, 2018

View reviewed changes

andralex added the Merge:auto-merge label Feb 20, 2018

Conversation

RazvanN7 commented Feb 12, 2018

Uh oh!

dlang-bot commented Feb 12, 2018 • 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!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

JinShil commented Feb 12, 2018

Uh oh!

RazvanN7 commented Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timotheecour commented Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 Feb 14, 2018 • 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!

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!

RazvanN7 Feb 14, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

timotheecour Feb 14, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dlang-bot commented Feb 12, 2018 •

edited

Loading

RazvanN7 commented Feb 13, 2018 •

edited

Loading

timotheecour commented Feb 13, 2018 •

edited

Loading

RazvanN7 Feb 14, 2018 •

edited

Loading

RazvanN7 Feb 14, 2018 •

edited

Loading

timotheecour Feb 14, 2018 •

edited

Loading

RazvanN7 commented Feb 14, 2018 •

edited

Loading