mozilla :: #jsapi

19 May 2017
00:08KWiersoluke: any chance you're actually around?
00:08lukeKWierso: i am
00:09KWiersohitting a merge conflict merging inbound to m-c after already merging autoland to m-c
00:09KWiersoboth patches reviewed by you :)
00:09lukeoh hah
00:09KWiersohttps://hg.mozilla.org/mozilla-central/diff/f25a37d7822e/js/src/wasm/WasmBinaryConstants.h and https://hg.mozilla.org/integration/mozilla-inbound/diff/c67aff0a734d/js/src/wasm/WasmBinaryConstants.h
00:10lukeKWierso: ah, right, NameSectionName moved to the end of the header
00:11KWiersoshould SourceMappingURLSectionName move with it?
00:11lukeKWierso: yeah, probably that'd be best
00:11KWiersothat was easy :)
00:11lukeKWierso: thanks for asking
00:19shuleobalter: ping
00:26shuleobalter: unping
00:41KiChjangcan i embed SM to an application that runs on android easily?
01:06shuleobalter: reping. i don't understand these new for-await-of tests at all
06:54jandemsstangl: what's the status of the gc scheduler?
08:39smaugjandem: have you measured speedometer when input.value is no-op or some such?
08:40jandemsmaug: not yet. We need to make sure that doesn't break the framework though
08:40smaugright
08:41jandemsmaug: how hard is it to prototype the "just change the text node" thing?
08:41smauglet me look
09:14Yoricshu: So, as I suspected, speccing decoding to ESTree is pretty simple. I have an advanced (though unfinished) spec here: https://github.com/Yoric/binjs-ref/blob/master/spec/CONTAINER.md
09:14YoricShould be future-proof wrt future versions of ESTree.
09:15evilpiejandem: i am not optimistic that for-in hasOwn will be a measurable win
09:15jandemevilpie: good to know
09:16jandemevilpie: btw did you measure octane earley-boyer with your arguments patch? it should be an improvement
09:17jandemevilpie: thanks for prototyping that btw, I think I'm going to refactor some things to allow stack-allocated type/payload
09:22evilpiejandem: I didn't, but I will try it when it get home
09:23jandemevilpie: cool. I probably won't get to that bug before next week
09:23jandembacklog of requests and patches to land
09:23evilpieYeah, I thought about fixing ValueOperand to support that, but it's a bit of work
09:24jandemyeah I want to rename ValueOperand to ValueRegister
09:25jandemthen rename useBox to useValueRegister
09:25jandemand then add useValue that allows stack-allocation
09:25jandemthat has some symmetry with use/useRegister
09:26jandemValueOperand is usually okay because the instruction needs the Value in a register
09:26jandembut this case is different
09:35evilpiejandem: are you sure we should go through that trouble? I feel like special casing this would be fine.
09:36jandemevilpie: special-casing how?
09:36evilpieJust add some code that loads tag/payload from stack/register
09:36evilpieSo treat them as two operands
09:37jandemyeah that was my plan
09:37jandemthe renames are not strictly necessary true
09:37jandembut at the same time it's easy. Will see :)
09:38jandemwe need this on x86 when the profiler is enabled, because we could have 3 Values + the length register
09:39jandemand we have 6 register available due to the profiler being enabled
09:39jandem(and loading all arguments into registers is pretty silly anyway)
09:40evilpieSure
09:41evilpieWith my approach we can choose a limit, maybe higher on x64
09:41jandemyeah I'd like to keep it the same to avoid big perf differences across platforms
09:42jandemI expected code to pass more arguments sometimes, but <= 3 covers almost everything, it&#39;s pretty sweet
09:55smaugjandem: do you have link to the speedometer tests
09:55smaugI wonder if they do anything with input&#39;s selection
09:56smaugif not, I have a patch for testing how badly .value handling affect to tests
09:56jandemsmaug: https://sm.duct.io/Full.html
09:57jandemsmaug: or https://github.com/WebKit/webkit/tree/master/PerformanceTests/Speedometer
09:57smaugjandem: how do I get that all downloaded?
09:57smaugthat
09:58jandemdoes the webkit repo work for you?
09:58jandemwe should upload a zip to bugzilla
09:58smaugI won&#39;t be running the tests
09:59smaugjust looking for the code
09:59jandemah ok
09:59smaugcan I easily download just speedometer part
09:59smaugnot the whole repo
10:00smaughmm, I&#39;ll upload my patch anyhow. It is only for testing
10:29smaugjandem: I&#39;m not getting much difference with or without my patch. 90 vs 94
10:29jandemsmaug: it does fix the micro benchmarks? not sure where that 15/20 number came from then
10:30smaugit is a hacky patch to make the microbenchmarks as fast as Chrome
10:31smaughmm, one random profile shows that creating frames takes lots of time
10:31smaugunder focus call
10:33smaughrm, the hack doesn&#39;t seem to work too well with actual speedometer
10:42smauger, hmm
10:42smaugdid I profile wrong build
11:47evilpiejandem: I think it might be 1000 point win on early-boyer, but I am not sure
11:47evilpiegoing to try on awfy
11:48jandemevilpie: nice
12:29evilpiejandem: https://arewefastyet.com/task_info.php?id=87797 looks like no change
12:35jandemevilpie: hm weird
13:01smaugwith the input.value hack + this one additional patch, speedometer is 119
13:01smaugbut Chrome 149
13:25jandemsmaug: still a pretty big win?
13:32smaugjandem: it is. I need to verify this and then figure out how to make the additional patch valid
13:33smaugin all the cases
13:33jandemsmaug: nice
13:35anbaHmm, how is it possible that IonBuilder::inlineRegExpSearcher is called with rxArg&#39;s not-equal to an object? RegExpSearcher is only called from self-hosted code, but according to gdb rxArg->resultType_ is MIRType::String...
13:45smaugjandem: bug 1366250
13:45firebothttps://bugzil.la/1366250 NEW, nobody@mozilla.org Flushing in nsFocusManager::CheckIfFocusable shows up significantly in Speedometer.
13:46jandemanba: we throw a TypeErorr if !IsObject(rx), but it&#39;s still possible to see a non-object type in IonBuilder, even if that RegExpSearcher call is unreachable
13:46jandemsmaug++
13:47smaugjandem: well, now I need to figure out how to fix the bug :)
13:48anbajandem: can we fix this somehow, for example by adding a return-statement? (We hit this issue in speedometer, which could explain the high number of non-inlined RegExpSearcher calls in bug 1365361)
13:48firebothttps://bugzil.la/1365361 NEW, nobody@mozilla.org Baseline: Optimize intrinsics used in self-hosted functions
13:49jandemsmaug: could we short-circuit if the element is already the one that has focus?
13:51jandemanba: yeah that&#39;s worth a try - the IonBuilder::improveTypesAtTest mechanism is supposed to handle this then
13:51smaugyeah, need to check what this is about
13:51smaugjandem: I doubt the element has focus there
13:51smaugin speedometer
13:51jandemsmaug: i think it has
13:51jandemsmaug: i think they use an html attribute in addition to the focus call
13:51jandemautofocus or something?
13:52jandem(not sure if that applies to all tests but I&#39;ve seen it on some)
13:53smaugjandem: ah, let me test
13:56smaugjandem: looks like blink does have such shortcut
13:56smaug if (GetDocument().FocusedElement() == this)
13:57smaug return;
13:58smaugbut focus isn&#39;t always set to the same element there in speedometer
13:59jandemsmaug: it&#39;s curious because the focus() calls in tests.js are in the prepare functions
13:59jandemi don&#39;t expect that to be timed
14:00jandemoh wait it uses .then so it&#39;s probably not under prepare
14:00smaugbut let me try with the same hack blink has
14:03smaugI&#39;m rather worried this will break browser chrome
14:11anbajandem: Adding a return-statement fixed the issue! I&#39;m now wondering if we should special-case ThrowTypeError in BytecodeEmitter to mark the function as always returning abruptly...
14:16jandemanba: we could probably also handle this in IonBuilder by terminating the block.. a bit annoying so maybe a BCE hack would be simpler
14:16jandemthe variable number of arguments passed to Throw* makes it a bit harder to do something nice
14:17smaugjandem: looks like from speedometer point of view it is enough to check if focus is already in the element. posted a patch to try
14:17jandemsmaug: nice
14:29anbajandem: Filed bug 1366263 for further investigation.
14:29firebothttps://bugzil.la/1366263 NEW, nobody@mozilla.org RegExpMatcher, RegExpSearcher, and RegExpTester not always inlined in Speedometer
14:59leobaltershu: I&#39;m looking at the for-await-of tests, anba filed an issue listing some. Let me re-check them
15:02jandemanba: this is great stuff, thanks for looking into it
15:04jandemanba: for the intrinsics where the actual type is always fixed (when the s-h code checks it first) I wonder if we should just remove these checks in IonBuilder, to avoid these issues
15:06evilpieI wonder why array_push is still so high, I thought we inlined that
15:57anbaevilpie: maybe it&#39;s something &quot;simple&quot;, like for example array_push is called with more than one argument. (With grep, I see multiple calls to a method named &quot;push&quot; with more than 1 argument in Ember code, but I don&#39;t know if that &quot;push&quot; method is actually array_push....)
16:05anbaevilpie: I did see multiple calls to inlineArrayPush with callInfo.argc() > 1, but I don&#39;t know if those array_push calls are in the hot path.
16:05evilpieanba: oh interesting, we should optimize that, six-speed also has that
16:05evilpieI have thinking about optimizing Object.assign and I think it&#39;s going to suck
16:06evilpieespecially because it uses Set and not DefineProperty, so we would have to make sure the property doesn&#39;t exist somewhere on the protochain
16:06anbaevilpie: yes, that&#39;s a real problem.
16:12jdmhow would you go about figuring out the cause of https://pastebin.mozilla.org/9022191 ?
16:13jdmtracing values in the nursery, and we come across one which has the wrong bits set
16:13* jdm wonders if rr would be the easiest solution here
16:14jdmwhat is the assertion verifying?
16:15jandemit means you have a Value that has a GC-thing type tag (object/string/symbol most likely), but a bogus payload
16:16jandemwhat is ptrBits?
16:16jandemthe assertion is verifying the pointer is aligned
16:16jandemsince all valid GC pointers have that alignment
16:19anbaleobalter: Placing iter.next() into then() looks correct to me.
16:23anbaevilpie: Thankfully it&#39;s easier to optimize CopyDataProperties from the object rest properties impl (uses DefineProperty and the target is always a plain object), so I hope that in the future more users will write |{...obj}| compared to |Object.assign({}, opt)|, and we can provide better performance more easily.
16:23evilpieanba: oh at least that is sane
16:26jdm(lldb) p/x ptrBits
16:26jdm(uint64_t) $10 = 0x0000000100000002 0x0000000100000002
16:26jdmjandem: that does look bogus
16:27jandemthe ptrBits assertion is infamous for catching memory corruption bugs, and these bugs are always serious
16:27jandemhm curious. Maybe use rr yes
16:28jandemcould be JIT code misbehaving
17:23sfinkjdm: yeah, rr sounds perfect for that. Run to the assert, set a hw breakpoint on that memory location, and reverse-continue to where the value got set.
17:24* jdm is deep in a yak pit of trying to enable vnc on a box that can run rr
17:25sfinkbleh. ssh in, run rr on it with DISPLAY=:0 ?
17:27jdm...
17:28sfinkI guess if you have to interact, that doesn&#39;t work so well
17:28* jdm is pretty sure that should work fine
17:36leobalteranba: thanks, I thought I missed something else from my interpretation of the specs
17:37jdmbah, I think I&#39;m just going to have to go into the office to run it in person
17:37jdmthat&#39;s annoyign
17:51shuYoric: cool, so what you have is currently completely parameterized over the actual tree grammar?
17:58shuYoric: that parameterization will be pretty convenient
19:03djvjscreen -r
19:13djvjjandem: are you coming to TO next week?
20:49shugandalf: ping
20:50gandalfpong
20:52gandalfTTL expired
20:52shugandalf: okay so... what is &quot;legacy&quot; in the context of legacy vs BCP47
20:53shugandalf: do all ICU APIs return legacy values, and all Intl stuff require returning BCP47 values?
20:53gandalfcontext url?
20:55gandalfgenerally, ICU returns non-legacy values and Intl as well, but both should support legacy language tags
20:57shugandalf: i&#39;m looking at anba&#39;s patch
20:57shugandalf: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1365650&attachment=8868650
20:57shugandalf: in the defaultCalendar function, it gets some value from ICU, then converts it to BCP47
20:59shugandalf: so am i understanding right that Intl *always* returns non-legacy
20:59gandalflooking
20:59anbashu: for example ICU returns &quot;gregorian&quot; for the Gregorian calendar, but Intl wants the new BCP value which is &quot;gregory&quot;
20:59shu...wat
20:59shu&quot;gregory&quot; really
21:00anbashu: &quot;gregorian&quot; is too long for Unicode extension subtags, so they had to shorten it to &quot;gregory&quot;
21:00shustandards are amazing
21:00anbashu: :-)
21:00gandalfanba: is that documented?
21:01gandalfhttp://icu-project.org/apiref/icu4c/ucal_8h.html#ac3da68a172e0dff2097004c811a49b13
21:01gandalfjust says it returns the keyword
21:01shuyes i saw that
21:02shuanba: could you also clarify the relationship between legacy values, BCP 47, and Unicode Technical Standard 35
21:03anbagandalf: Breaking News - ICU&#39;s documentation sometimes lacks info.
21:03gandalfanba: ugh, I know, but is it documented anywhere?
21:03gandalfdid you just get to it via trial&error?
21:04shuanba: well, so, my concern as a reviewer is this -- i implicitly trust that you are correct, because based on historical data the chances of you getting things wrong wrt standards is exceedingly small. i&#39;m trying to figure out what the risks are if i just rubberstamp this
21:04gandalffound this - https://sourceforge.net/p/icu/mailman/message/32768651/
21:05gandalfthis is nuts that this is the only thing you can find
21:05anbagandalf: In https://hg.mozilla.org/mozilla-central/rev/c07319097f3b76aa6fe758a0246d2d2958eea717, I&#39;ve updated Intl.cpp to use uloc_toUnicodeLocaleType (http://www.icu-project.org/apiref/icu4c/uloc_8h.html#a0261e5fbb0ea96e16d111b681426f2a4) to get the correct BCP 47 value
21:06gandalfyeah, saw that. I&#39;m just amazed how little there is on the Internet about this keyword type conversion
21:07gandalfseems like there&#39;s no url to even link in the comment in the code :(
21:08anbashu: the new method has the same code as the existing availableCalendars method http://searchfox.org/mozilla-central/source/js/src/builtin/Intl.cpp#2704-2723
21:08anbashu: or rather it&#39;s a subset of availableCalendars
21:08shuanba: ah great that helps
21:11anbagandalf: I read about it somewhere, unfortunately I can no longer remember where it was. It could be ICU4C api docs or source, or ICU4J api docs or source code, or ICU website documentation, or ICU bug tracker, or ... :-/
21:11gandalf:(
21:12eogerI&#39;m getting a &quot;TypeError: X is not a function&quot; if I put the async keyword before a function name (object shorthand syntax)
21:12eogernot sure if this has been reported
21:13shueoger: full string that doesn&#39;t parse?
21:14eogerlemme try to come up with a minimal test case
21:14shuthanks
21:41eogerSorry I couldn&#39;t come up with that test case :/
21:41eogerthere&#39;s &quot;catch&quot; in the function name, that could be related
21:44gkwshu: ping about bug 1366399
21:44firebothttps://bugzil.la/1366399 NEW, nobody@mozilla.org Crash [@ JS::Rooted<jsid>::registerWithRootLists] or Assertion failure: isObject(), at dist/include/
21:44gkwshu: it happened after the test262 test landed and blew up one of the fuzzers
21:44gkwI&#39;m still trying to bisect
21:56shugkw: i&#39;m gonna bet it&#39;s a dup of bug 1364608
21:56firebothttps://bugzil.la/1364608 NEW, shu@mozilla.com Stash rval in AsyncIteratorClose
21:56shugkw: which i haven&#39;t landed due to some possibly incorrect test262 tests, but i could land that and keep the test262 tests disabled in the meantime
21:56shugkw: i got a big backlog today, can&#39;t promise i&#39;ll get to it before end of the day
21:57gkwshu: can we backout the test262 tests first? I don&#39;t think the fuzzer cares about enabled/disabled
21:57gkwit just searches the jit-tests directory
22:00shugkw: hm, i&#39;d rather not back out test262
22:00shugkw: let me finish this review and confirm whether it&#39;s a dupe or not
22:01shugkw: if so, i&#39;ll land fix with tests still disabled, if not dupe, we can talk abackout
22:06gkwshu: thanks
22:43shugkw: confirmed dupe, will land fix when local tests finish
22:43gkwshu: perfect,thanks!
23:26shuJohn-Galt: ping
23:26John-Galtshu: pong
23:26shuJohn-Galt: please explain this template type to me from the patches
23:26shuhttps://irccloud.mozilla.com/pastebin/Pa1ZZule/
23:27John-Galtshu: Basically, it checks that the argument is a function (or lambda) that accepts a ParseTask* and returns a bool.
23:29shuJohn-Galt: what&#39;s the second argument to EnableIf?
23:29John-Galtshu: There&#39;s no second argument to EnableIf, just to IsSame.
23:30shuJohn-Galt: what does EnableIf<T> mean then?
23:30John-Galtshu: If you want, I can write a helper for TypeUtils.h to make checking function types a bit cleaner.
23:30John-Galtshu: It&#39;s basically a gigantic hack to cause a substitution failure if its argument evaluates to FalseType
23:30shuJohn-Galt: err, is it EnableIf<T> or EnableIf<bool> ?
23:31shuJohn-Galt: sure, but i&#39;d like to understand it :)
23:32John-Galtshu: It&#39;s EnableIf<bool>, and when <bool> is true, EnableIf::Type evaluates to TrueType
23:32John-GaltWhen it&#39;s not, EnableIf::Type isn&#39;t defined, and there&#39;s a substitution failure, so the template doesn&#39;t match
23:32shuJohn-Galt: ah ha, ok
23:33shuJohn-Galt: and that decltype says, what&#39;s the return type of F if i passed it a ParseTask*
23:33John-Galtshu: Yup
23:34shuokay i think i got it, thanks
23:34John-Galtnp
23:36sfinkvery similar to my monstrosity at http://searchfox.org/mozilla-central/source/js/src/jscntxtinlines.h#134
23:41shuTIL you can have unnamed template parameters
20 May 2017
No messages
   
Last message: 3 days and 4 hours ago