Page MenuHomePhabricator

Anonymous Functions run into evaluator recursion limits pretty quickly
Open, In Progress, HighPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

It evaluates an anonymous function that has no arguments, because they have been bound in the two inner functions. The function is a simple subtraction.

What happens?:

Error message: Reached recursion limit in evaluator (K1: "WASM interpreter aborted (unreachable executed)")

What should have happened instead?:

I would have hoped it would evaluate :D

If I change the order of the calls -- i.e. I first change the first (instead of the second) argument, and then the first again -- it works.

Software version (on Special:Version page; skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):

Event Timeline

The problems seem to start even sooner: this is just binding a value to the second position of subtraction. The "arguments" key remains unevalued, i.e. it says

filter with second common element (argument declaration has not key, arguments of an anonymous function (anonymous function), argument id)

instead of giving the evaluated values.

Reminds me of T431734 but different

Z38676 (bind value by position) is implemented by Z38679 which uses Z38598 (bind value by id) which is implemented by Z38630 which uses Z38619 (substitue s-expression) which is implemented by Z38629 which uses Z38625 (is argument reference) which is implemented by Z38628 which uses Z19352 (object has this type) which is implemented by Z31193 which uses Z22764 (string from type)

(and Z22764 seems to be the culprit in the evaluator)

Here is the function call that produces the not fully evaluated object:

call to bind by position

Here is the function call that produces the not fully evaluated object:

call to bind by position

Ugh, when I run this locally, I get the same WASM: overflow terrible etc. error. I'm not sure how my environment differs from prod. Continuing to investigate!

I am including here an example of an evaluator request that causes the WASM interpreter aborted (unreachable executed) error. You will notice that there's a TREMENDOUS amount of extra content in here, most conspicuously the labels in dozens of languages. We shouldn't have to process this much stuff just to add two numbers together. On the other hand, we can't just strip the labels out, because some functions rely on the labels to do NLG (a thing I have complained about before, often). There is no way to reconcile this situation; we simply need to find a different way to model data here.

Okay, things are becoming clear.

The unresolved Z18 in "call to bind by position" was due to the bug where errors in realizeTopLevel weren't being propagated correctly. That's been fixed as of today's deployment, so now my local behavior and prod behavior are identical.

When I hack around the crazy number of labels, that call actually works locally, so there may not be a problem with bind by position. "When I hack around" is a VERY big deal, though; we really need to find an answer for that issue or we'll simply not be able to run functions in the evaluator anymore.

The enclosing call does still fail. This puts me back on the trail of a scope issue.

some functions rely on the labels to do NLG (a thing I have complained about before, often)

Are we talking about labels for ZIDs or for QIDs? If the former, I think we have/are moving away from using them in NLG. There was initially a question about whether we should get month names from the WF ZID, but IMO we're much better off mapping to the WD QID then getting labels from there, because it's much better maintained.

TLDR: I personally wouldn't mind if the ZID labels were unavailable to functions.

This is purely

some functions rely on the labels to do NLG (a thing I have complained about before, often)

Are we talking about labels for ZIDs or for QIDs? If the former, I think we have/are moving away from using them in NLG. There was initially a question about whether we should get month names from the WF ZID, but IMO we're much better off mapping to the WD QID then getting labels from there, because it's much better maintained.

TLDR: I personally wouldn't mind if the ZID labels were unavailable to functions.

! If this is true, it's fantastic. It looks like both of the issues linked trace to the label profligacy problem. If we can completely avoid labels in evaluator requests, I think we'll be much better off.

I've done a little more investigation, and am even more convinced that Z20607 is very little used. I've posted on Telegram for confirmation.

Ha! One of the two uses was made very recently by @YoshiRulz for another phab T355738. Although cute, I think we could sacrifice this for speed, as the same thing could be done from external tools/queries.

It looks to me like this is a Z3K3 problem rather than Z2K3. The Z3K3 getter is Z23323 and this has no uses apart from one external test case: Z23326. I’m not seeing any Z3K3 references in code, so I think it is fair to conclude that Z3K3 currently has no NLG uses.

As with Z2K3, I think any NLG uses of Z3K3 should be mediated by Z828 (or equivalent builtin) but there are still some outstanding bugs to be fixed to make Z828 operational again.

I don't care if that function breaks, it was a PoC.

edit: See also T343685 re: using labels from WD instead of WF.

DSantamaria changed the task status from Open to In Progress.Tue, Aug 11, 3:46 PM