1367878 - RegExp handling shows up in Facebook profile

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

https://perf-html.io/public/b502ce979fd1a7f3b41d57203195ced77f2583ff/calltree/?range=23.4688_26.5325&search=irregexp&thread=5

Comment 1

[Nicolas B. Pierron [:nbp — off until 29-09]]

js::irregexp::CompilePattern takes 133ms (mostly js::irregexp::*::Emit), and as far as I know, this code is not interruptible as of today.

I see 2 "simple" ways to approach this problem, which both have pitfalls:
 1/ We could add checks to call the interruption callback in the *::Emit functions, such that we can stack event loops, but we would add JS race issues in facebook.
 2/ Quantum DOM would save us by making sure we can switch between multiple threads, but this would not solve the 133ms facebook-only pause.

Does this issue exists also in Chrome, or did they changed irregexp to better handle this parsing issue?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Bobby, do we have some way to capture some kind of profiles on Chrome?

Comment 3

[(no longer active)]

A native profiler should be usable to profile Chrome manually if someone wants to verify whether they hit the same irregexp code or not.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Last time I tried, Chrome builds didn't have symbols included.

Comment 5

[Bobby Chien]

(In reply to Olli Pettay [:smaug] from comment #2)
> Bobby, do we have some way to capture some kind of profiles on Chrome?

Mike, could you help Olli to capture the profiles? Thanks.

Comment 6

[Mike Lien[:mlien]]

(In reply to Bobby Chien [:bchien] from comment #5)
> (In reply to Olli Pettay [:smaug] from comment #2)
> > Bobby, do we have some way to capture some kind of profiles on Chrome?
> 
> Mike, could you help Olli to capture the profiles? Thanks.

We only have Chrome's tracing log, e.g., https://tinyurl.com/mzen5lo, and just like Olli mentioned it doesn't have symbols.

Comment 7

[Ting-Yu Chou [:ting] (away)]

71ms was spent in js::irregexp::NativeRegExpMacroAssembler::BindBacktrack which has a linear search, this seems something we can improve. I'll check how it behaves usually.

Comment 8

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #7)
> 71ms was spent in js::irregexp::NativeRegExpMacroAssembler::BindBacktrack
> which has a linear search, this seems something we can improve. I'll check
> how it behaves usually.

Hmm, I don't see this symbol from the profile that I just captured on Nightly 20170710100245.

Comment 9

[Cervantes Yu [:cyu] [:cervantes]]

I didn't see this in either Firefox nightly or in Google Chrome profile (by searching for regexp in the profiled native stack). Also given the reported number in bug 1366960 (76% = 172 ms), I think the regular exception processing happens long *after* the chat tab is opened and doesn't slow down the opening of the tab. It does jank the browser, but it's a separate issue. We should unblock 1366960.

Comment 10

[Bobby Chien]

unblock per comment 9.

Comment 11

[Kannan Vijayan [:djvj]]

(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> js::irregexp::CompilePattern takes 133ms (mostly js::irregexp::*::Emit), and
> as far as I know, this code is not interruptible as of today.
> 
> I see 2 "simple" ways to approach this problem, which both have pitfalls:
>  1/ We could add checks to call the interruption callback in the *::Emit
> functions, such that we can stack event loops, but we would add JS race
> issues in facebook.
>  2/ Quantum DOM would save us by making sure we can switch between multiple
> threads, but this would not solve the 133ms facebook-only pause.
> 
> Does this issue exists also in Chrome, or did they changed irregexp to
> better handle this parsing issue?

Would it be possible to do the irregexp jit-compile in the background and use the VM-based regexp impl until then?  Siimilar to how we background Ion-compilation?

Comment 12

[Naveed Ihsanullah [:naveed]]

Sean can you take a look to see if there is something we can do here? Do we need to update our fork from Chrome's

Comment 13

[Jan de Mooij [:jandem]]

We should first check if this is still a problem; I fixed a number of regex-related perf issues this year. If it still shows up we can look at a profile to see where we're spending our time.

Comment 14

[Matthew Gaudet (he/him) [:mgaudet]]

We've updated our regexp import (multiple times since), and 5 years is a long time in Facebook time... if it shows up again, let's open a new bug.