WIP: Ability to call overridden functions from the std library #798

r-l-x · 2018-04-20T09:40:33Z

Gopherjs overrides some functions from the go standard library. The original implementation from the standard library gets removed from the compilation process.

This PR implements the ability to retain designated overridden functions from the go standard library.
#558 is a typical use case for this (try an optimized way to get the result, but if this doesn't work, fallback to the standard library).

Here is how to use it:
If a function in compiler/natives overrides a function from the standard library and has the gopherjs:keep_overridden directive in the function comment, then the overridden function from the standard library will be compile, and can be called under the name _gopherjs_overridden_<function_name>.

A typical use would look like (in a file in compiler/natives):

//gopherjs:keep_overridden
// This function overrides foo() from the standard library
func foo() {
  // try the optimized function
  if err := optimized_foo(); err != nil {
    // this didn't work, fallback to the standard library implementation
    _gopherjs_overridden_foo()
  }
}

myitcv · 2018-04-20T10:24:32Z

@r-l-x - can you update the example in the description to be concrete, i.e. a real situation where we would use this? I'm struggling to understand how/where this would be used (sorry, probably being slow). Thanks

myitcv · 2018-04-20T10:26:03Z

Ah, presumably https://github.com/r-l-x/gopherjs/blob/bcfda31b2bc688a9dce31c1402629ad44d949d14/compiler/natives/src/crypto/sha256/sha256.go#L28-L38 is such an example?

r-l-x · 2018-04-20T10:33:12Z

@myitcv Yes exactly.

myitcv · 2018-04-20T10:37:32Z

My suggestion would be we use this directive for at least one function as part of this PR to demonstrate it actually works, passes tests, minifies properly etc.

Then we can add additional functions etc as proposed in #558 and anything else in future PRs.

myitcv

Adding as a review

My suggestion would be we use this directive for at least one function as part of this PR to demonstrate it actually works, passes tests, minifies properly etc.

myitcv

Adding as a review

My suggestion would be we use this directive for at least one function as part of this PR to demonstrate it actually works, passes tests, minifies properly etc.

myitcv · 2018-04-20T10:41:27Z

Sorry for the noise in my last few comments/reviews...

r-l-x · 2018-04-20T11:46:08Z

Well, actually, #558 is the prefect exemple for this (and it's based on this branch). @shurcooL suggested issuing a separate PR for this, so that's what I did, but if you prefer considering #558 directly, I don't mind.

It's not easy to add a "minimalist" use for this feature without risking breaking something or adding useless code.

myitcv · 2018-04-20T12:13:02Z

It's not easy to add a "minimalist" use for this feature without risking breaking something or adding useless code.

I think this is the key. Either there needs to be a test or some minimal use of what's being committed in this PR, some way we can "know" it's working.

r-l-x · 2018-04-20T23:47:31Z

Ok, I just used this in the gopherjs regexp test "TestOnePassCutoff".

This test was systematically skipped, because apparently it may raise "Maximum call stack size exceeded" errors. I rewrote the test using gopherjs:keep_overridden, so that the original test is run and only in case of a panic, the test gets skipped. This allows the person running the test to know if the test was actually in success or not, without making the gopherjs tests fail in case of error. It wouldn't be possible to do this without this PR (appart from duplicating the code from the standard library).

And actually both on my configuration and on circleci the test runs well (so it seems like it could be re-enabled):

=== RUN   TestOnePassCutoff
--- PASS: TestOnePassCutoff (0.08s)

myitcv · 2018-04-21T12:41:35Z

Ok, I just used this in the gopherjs regexp test "TestOnePassCutoff".

Perfect.

And actually both on my configuration and on circleci the test runs well (so it seems like it could be re-enabled):

Thanks for flagging. I suspect this got fixed by #687. I've raised #801 to follow up on re-enabling this and other tests.

myitcv

Minor proposed fix.

myitcv · 2018-04-21T12:53:52Z

build/build.go

+		return false
+	}
+	for _, comment := range doc.List {
+		text := comment.Text


Might I suggest the following is slightly clearer in its intention?

text := strings.TrimRightFunc(comment.Text, unicode.IsSpace) if text == "//gopherjs:keep_overridden" { return true }

i.e. you're looking for a line that contains only the special comment, save any trailing space.

I actually took this part from the go pragma directive parser:
https://github.com/golang/go/blob/master/src/cmd/compile/internal/gc/noder.go#L1435
It seems like you can add text after a go pragma directive, and it will be ignored, and I think it's a good idea to do the same here.

Got it. I think you'll find that is effectively the parse phase of the compiler; so it has to deal with all types of pragma. Many of which take arguments.

In this case we've already parsed into the AST and we know we have a comment group.

And specifically, I'm guessing you want //gopherjs:keep_overridden to be used without arguments, because there are none. Hence it doesn't make sense for anything to follow that special comment.

Considering the pragma function from the standard library, my understanding is that pragmas expecing arguments (go:linkname, go:cgo_*) are treated in the non-default cases of the switch. The default case, from which this code is taken, only treats pragmas with no arguments (go:nointerface, go:noescape, etc., see the implementation of pragmaValues)
So it looks like the standard library allows putting any text (so basically a comment) after a pragma without argument. I think it's appropriate to have the same behaviour here (even if I agree that it's not the typical use case).

So it looks like the standard library allows putting any text (so basically a comment) after a pragma without argument. I think it's appropriate to have the same behaviour here (even if I agree that it's not the typical use case).

You're absolutely right, thanks for correcting me.

I'm easy either way; in this PR I think you can be very specific and prescriptive because you're defining the pragma so it's in your gift to do what you want 😄 - to my mind doing so keeps the documentation (on how to use your pragma) simple as well as the implementation.

But, as I said earlier, it's a minor point in any case.

Before this commit, every overriden standard library function was renamed '_'. Now, if there is the following directive in the function comment: "//gopherjs:keep_overridden", they are prefixed with "_gopherjs_overridden_" so that they can be called from the overriding function. This is typically useful to use the standard library implementation as a fallback.

Before this commit, TestOnePassCutoff was skipped because a 'Maximum call stack size exceeded' message may occur. Now we use gopherjs:keep_overridden to try the original test, and skip only if the original test failed.

r-l-x · 2018-04-21T18:08:51Z

I just rebased to fix conflicts and fixed a typo in the code.

myitcv · 2018-04-22T19:00:59Z

This LGTM 👍

nevkontakte · 2021-10-23T17:53:50Z

Hi @r-l-x, I'm finally getting through the backlog of issues and pull requests, sorry it took this long. This seems like a very useful change, would you be able to rebase it on top of the current master?

r-l-x · 2021-10-28T10:45:37Z

Hi @nevkontakte, when trying to rebase this, I saw that a new gopherjs:prune-original directive was introduced, and I think my PR needs some updates to be consistent with this keyword (typically I should probably name my directive gopherjs:keep-original instead of gopherjs:keep_overridden)

nevkontakte · 2021-10-30T13:57:54Z

@r-l-x yes, renaming to gopherjs:keep-original sounds good to me. If you don't think you'll have to do the rebase in the near future, please let me know, I can try to do it myself.

... as per #798 (comment)

flimzy · 2023-06-27T07:55:31Z

Rebased and merged as #1216

r-l-x mentioned this pull request Apr 20, 2018

WIP: WebcryptoAPI support #558

Open

r-l-x changed the title ~~Ability to call overridden functions from the std library~~ WIP: Ability to call overridden functions from the std library Apr 20, 2018

r-l-x force-pushed the keep_overridden branch from 27a4b71 to 24a2d2d Compare April 20, 2018 09:49

r-l-x changed the title ~~WIP: Ability to call overridden functions from the std library~~ Ability to call overridden functions from the std library Apr 20, 2018

myitcv suggested changes Apr 20, 2018

View reviewed changes

myitcv mentioned this pull request Apr 21, 2018

test: explore re-enabling tests previously skipped because of stack size #801

Open

myitcv reviewed Apr 21, 2018

View reviewed changes

r-l-x and others added 3 commits April 21, 2018 20:02

regexp test: added an example use for gopherjs:keep_overridden

1baef4b

Before this commit, TestOnePassCutoff was skipped because a 'Maximum call stack size exceeded' message may occur. Now we use gopherjs:keep_overridden to try the original test, and skip only if the original test failed.

compiler/natives: regenerate

4bcab8f

r-l-x changed the title ~~Ability to call overridden functions from the std library~~ WIP: Ability to call overridden functions from the std library Apr 21, 2018

fixed typo

21522a8

r-l-x force-pushed the keep_overridden branch from da8be1b to 21522a8 Compare April 21, 2018 18:07

r-l-x changed the title ~~WIP: Ability to call overridden functions from the std library~~ Ability to call overridden functions from the std library Apr 21, 2018

nevkontakte self-assigned this Oct 23, 2021

nevkontakte added the WaitingForInfo label Oct 23, 2021

r-l-x changed the title ~~Ability to call overridden functions from the std library~~ WIP: Ability to call overridden functions from the std library Oct 28, 2021

flimzy mentioned this pull request Jan 3, 2022

compiler/natives/src/strconv: Use js string conversion utilities to improve performance #1101

Merged

flimzy mentioned this pull request Jun 26, 2023

Ability to call overridden functions from the std library #1216

Merged

flimzy added a commit that referenced this pull request Jun 26, 2023

Rename gopherjs:keep_overridden to gopherjs:keep-original

fe0510d

... as per #798 (comment)

flimzy closed this Jun 27, 2023

flimzy mentioned this pull request Jun 27, 2023

Investigate native JS implementation for ParseInt where permitted #1218

Open

WIP: Ability to call overridden functions from the std library #798

WIP: Ability to call overridden functions from the std library #798

Uh oh!

Conversation

r-l-x commented Apr 20, 2018

Uh oh!

myitcv commented Apr 20, 2018

Uh oh!

myitcv commented Apr 20, 2018

Uh oh!

r-l-x commented Apr 20, 2018

Uh oh!

myitcv commented Apr 20, 2018

Uh oh!

myitcv 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!

myitcv left a comment

Choose a reason for hiding this comment

Uh oh!

myitcv commented Apr 20, 2018

Uh oh!

r-l-x commented Apr 20, 2018

Uh oh!

myitcv commented Apr 20, 2018

Uh oh!

r-l-x commented Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myitcv commented Apr 21, 2018

Uh oh!

myitcv left a comment

Choose a reason for hiding this comment

Uh oh!

myitcv Apr 21, 2018

Choose a reason for hiding this comment

Uh oh!

r-l-x Apr 21, 2018

Choose a reason for hiding this comment

Uh oh!

myitcv Apr 21, 2018

Choose a reason for hiding this comment

Uh oh!

r-l-x Apr 21, 2018

Choose a reason for hiding this comment

Uh oh!

myitcv Apr 22, 2018

Choose a reason for hiding this comment

Uh oh!

r-l-x commented Apr 21, 2018

Uh oh!

myitcv commented Apr 22, 2018

Uh oh!

nevkontakte commented Oct 23, 2021

Uh oh!

r-l-x commented Oct 28, 2021

Uh oh!

nevkontakte commented Oct 30, 2021

Uh oh!

flimzy commented Jun 27, 2023

Uh oh!

Uh oh!

myitcv left a comment •

edited

Loading

r-l-x commented Apr 20, 2018 •

edited

Loading