mozilla :: #jsapi

15 Mar 2017
00:03jimbDoes anyone understand mozbuild well enough to explain why mozglue gets included in the JS shell link?
00:03jimbThere doesn't seem to be any documentation on mozbuild at all.
00:04njnjimb: I bet you $5 glandium can answer your question
00:04glandiumjimb: mozglue contains mfbt and jemalloc. You can't not link it
00:04jimbglandium: So it's hard-coded into mozbuild?
00:05glandiumjimb: yes and no. It's implied by "GeckoProgram"
00:05jimbWhat does this do, then?
00:05jimbglandium: But "implied by GeckoProgram" is what I needed. Thanks!
00:07glandiumjimb: oversight, it's redundant
00:07glandiumjimb: feel free to file a bug to remove it :)
00:08jimbI pride myself on never being simply confused. I get *deeply* confused.
00:08jimbglandium: Okay, great.
00:35jimbglandium: Bug 1347360. (Got distracted by dog, who wanted to play with my wife's piano students' shoes.)
00:35firebot NEW, browser/app/ has unnecessary USE_LIBS directive
00:46shui hate iteratorclose
00:46shumy word
02:31shuoo hit a MOZ_CRASH at JitFrames.cpp:666
02:31shuv spooky
03:01mrgigglesthe mozilla-inbound tree is now closed (Bustage)
03:07mrgigglesthe mozilla-inbound tree is now open
03:08WaldoI can't seriously be pondering virtual inheritance for this
03:09Waldomaybe this is a sign I should stop for the day
03:48djvjnjn: pong
03:48njndjvj: hi
03:49djvjnjn: what's up?
03:49njndjvj: just wondering about the review request in bug 1345262
03:49firebot NEW, Assertion failure: activeContext() == js::TlsContext.get()
03:49djvjnjn: right, started looking at tha today. didn't finish.
03:49njnare you the right person to look at that?
03:49djvjme or shu
03:49njnok, great
03:49djvjbut I can take it.
03:50njnif you started looking today, sounds like it's not far off
03:50djvjsorry, I have kind of become rusty with remembering review work over the last year or so..
03:50njnthe assertion is very frequent so it will be good to get fixed
03:50djvjnjn: I can r? now.
03:50njngreat, thank you
03:51djvjnjn: np. I should have seen it and finished it earlier.
03:56djvjnjn: nice cleanup and commenting..
04:14djvjnjn: r+
04:14njndjvj: thank you
04:15njneveryone loves a good state machine comment
04:15djvjnjn: they're valuable.
04:16djvjthat said, the code otherwise made it reasonably clear what was going on.
04:16djvjI was thinking of commenting on considering splitting the "intent" state from the "actual" state, but what you have implemented works perfectly fine.
04:18djvjnjn: what are you working on these days?
04:18djvjprofiler, clearly
04:18njndjvj: yeah, lots of profiler stuff, mainly just fixing broken stuff and cleaning up messy stuff
04:18njnthere has been plenty of that to do
04:18djvjnjn: yes, certainly
04:19njnheaps of global structures were badly racy, I fixed that
04:19djvjnjn: that's awesome
04:19njnand generally trying to make it not crash/assert if you use it for more than 1 minute at a time
04:20djvjnjn: I didn't realize it had gotten that bad..
04:20njnit was pretty bad! esp. if you profile worker threads
04:20njnthe default is just GeckoMain+Compositor, it wasn't too bad in that configuration
04:20djvjoh right.. worker threads were basically a wasteland back then too.
04:20njnbut turn on workers and boom!
04:21djvjhey, so we have this problem with scanning the jitcode map during gc
04:21djvjI guess it's more of a gc problem than a profiler problem, really
04:22djvjwe can't escape having those weak pointers in the table.
04:22* njn is curious but ignorant
04:24djvjso as part of the jit coach stuff, there's a table that annotates jitcode, used by the profiler at serialization time.
04:25djvjbasically we package up and send out a bunch of extra annotation about the jit stack (optimizations active at particular sites0
04:25djvjwait, you should be familiar with all of this..
04:26djvjanyway, the table that tracks that info has weak pointers into the js heap (pointers to JSScript*s amont other things)
04:26djvjthat needs to be updated every gc
04:26djvjand it's taking a long time
04:26djvjso looking at gc slices during profiling is compromised due to skew
04:26djvjthe thing is, I don't think we're using any of this information on the frontend.
04:27djvjwe could just turn it off in the short term and eliminate that issue.
04:28djvjhmm. well, I should sleep.
05:53shudjvj: i was going to look at it eventually but i haven't had time
05:53shudjvj: the minor gcs are causing skews i think
05:54djvjthe thing is, for the super short term (say, few months), we could afford to just disable it and not suffer too much bitrot
05:54djvjI mean, there's no frontend for the data.. who is using it at this point?
05:55djvjshu: thoughts?
05:56djvjshu: honestly, I think enabling the backend for the jit coach infrastructure (since it incurs memory, gc, and cpu overhead), should be gated on there being a plan for a UI..
05:58djvjwtf am i still awake
06:00shudjvj: for jit coach backend, i agree
06:01shudjvj: but for this particular skew, it's minor gc on the whole table
06:01shudjvj: the jitcoach info doesn't help matters, certainly
06:01shudjvj: we can see how much it improves with just removing the jitcoach parts
06:01djvjshu: the table is only needed to support jit coach right?
06:01djvjshu: oh wait no
06:01djvjwe do lazy stringification via that
06:01shudjvj: no, the rest of the table needs stuff too
06:01shudjvj: yeah, the only thing that goes in the buffer is the jitcode pc
06:01shudjvj: so we keep stuff alive
06:02djvjshu: someone mentioned there being a better mechanism to do this in the GC now
06:04shudjvj: oh really? let's chat later, go to bed
06:05djvjroger. good night.
08:29lthmrgiggles: is mozilla-inbound open?
08:29mrgiggleslth: mozilla-inbound: open
08:51crowbotms2ger: sfink said yes, you can put mrgiggles in any channel you like. "mrgiggles: want to hang out in XXX?" will work. I think "!invite XXX" would work too.
08:52Ms2gersfink, good to know, thanks
10:50nbpnaveed: first, these results are might not be totally representative, as they basically serialize everything if we happen to wait on the parser threads, which is likelyy to be the case, as these numbers are less noisy than the others. Also this results basically moved all parser task to the other threads. This results were good to highlight that the current hypothesis was not enough, and to show the trends
10:51nbpof results, but they are not final.
10:53nbpnaveed: For the first run optimization, I am not sure there is much we can do. We can potentially move the problem to the second run, but this would still be the same problem.
10:57nbph4writer: I have no precise idea why the first example first run is hit that badly, but I would not look too much into these results. The fact that these results are less noisy than others is probably due to the fact that we wait for the parser task which are all put off-main-thread with the pref that I added. So I would not investigate more than that before fixing the issue.
10:58nbph4writer: I also think that I should probably land asap behind a pref, and improve after.
10:58h4writernbp: I agree that we should land behind a pref
11:00h4writernbp: eventually we should make sure it doesn't regress the first iteration that much
11:00nbph4writer: I think this is a worse case, but I don't think this is avoidable.
11:00nbph4writer: the bytecode cache is a trade-off.
11:01nbph4writer: as I mention above, maybe we could move it to the caching to the second run, but I don't think it matters much.
11:01h4writernbp: still 25% seems a lot.
11:01nbp^ move the caching on the second run instead of the first
11:02nbph4writer: or I can just stop working on this "optimization"
11:03h4writernbp: we shouldn't just stop, without trying and to see why we have a 25% regression there !
11:03h4writernbp: what I'm saying is that I can agree that this is a worst-case behaviour and we should look into and see if we can decrease that a bit
11:04h4writernbp: that it will hurt the first run a little bit, that is normal.
11:04nbph4writer: as I said, I don't care about the "why" of the current results, as they are most likely related to the custom config made by a pref that I added for testing purposes.
11:05h4writernbp: before we can enable it we should try to get actual numbers to know for sure
11:05nbph4writer: what is inbteresting about the current results, is that doing the bytecode cache on only the script which are parsed out of the main thread is not enough to bring any improvement.
11:05Yoricstandups: Trying to understand how to visualize the results of recent Telemetry probes.
11:05standupsOk, submitted #43733 for
11:05nbph4writer: what is interesting is also to see the worse/best regression & improvements possible.
11:06h4writernbp: right. The numbers after the cache are encouraging :D
11:06h4writernbp: how did you infer the "the bytecode cache on only the script which are parsed out of the main thread is not enough to bring any improvement."
11:07nbph4writer: because that's how it is implemented today.
11:07nbph4writer: and it sounded like an easy to implement and a reasonable assumption to make.
11:08h4writernbp: I don't understand. We do see an improvement with bytecode cache, right?
11:08nbph4writer: I had to make a preference to make the test case work, and the preference force the bytecode cache by forcing all scripts to be parsed out of the main thread.
11:09nbph4writer: I had to turn this pref on, to get the results which are listed in the bug comment.
11:10h4writernbp: but you didn't list any numbers with bytecode cache and without forcing all scripts to be parsed out of the main thread, right?
11:10nbph4writer: indeed, I did not, because they were mostly identical.
11:10nbph4writer: I was not able to notice any major differences
11:11h4writernbp: and we don't do bytecode cache for items on the main thread?
11:11nbph4writer: not yet.
11:11nbph4writer: this would imply changing the way our EvaluateScript function works.
11:12* nbp office lunch in a few minutes
11:12h4writernbp: does it mean we don't run enough scripts on "the off thread" currently?
11:13nbph4writer: the most I saw, was 3, but the average is roughtly 0.8 scripts are ran out of the main thread per pages
11:13nbph4writer: That would be something to test indeed, to see if we should do more off-thread parsing.
11:13h4writernbp: and second question. Can you give numbers with the preference to force all scripts to be parsed out of the main thread without bytecode cache?
11:14nbph4writer: This is the left colum.
11:15* nbp lunch
11:15h4writernbp: Ah now I understand. I thought that the left column was without any patch (and no forcing to offthread)
11:15h4writernbp: bon apptit
12:17tilljandem: ping
12:17jandemtill: pong
12:17tilljandem: I'm trying to write jsapi tests for ReadableStream, and ran into a Promise-related issue
12:18tilljandem: specifically, you can't use promises if the embedding doesn't register hooks for enqueueing promise jobs. We do that in the shell, but not in the jsapi harness
12:20jandemtill: right
12:21tilljandem: so I can either duplicate all that code from js.cpp - and it is quite a lot of code - or do something less annoying
12:21jandemmaybe add a shell function?
12:21tilljandem: I *thought* providing an internal mechanism inside the engine that could optionally be used would be that less annoying option
12:22tillfor the jsapi stuff I need to test?
12:22jandemoh it's just for the jsapi part
12:22jandemtill: you mean have the engine provide a default implementation that we can then use in the shell/jsapi-tests?
12:23tilljandem: yeah, and use that for the shell, too. Other embeddings that don't need or have their own event loop could just use that, too
12:23tilljandem: that'd work fine, too, if it weren't for error reporting
12:24tilljandem: the way Promise jobs are processed, errors are discarded in the iteration over the queue. So I'd need to report them from inside the loop. But since your refactoring to always have the embedding report exceptions, that's not all that easy anymore
12:25tilljandem: hrm, now lunch arrived :( Will re-ping later, sorry
12:25jandemtill: ok np
13:31tilljandem: ok, back "Bug slim-fast" is blocked by "Bug unsalted-butter", blocked by "Bug butter-delivery", blocked by "Bug js-startup-cache".
13:33tilljandem: so one thing I considered was to require calling JS::RunJobs in a loop until it returns true, reporting exceptions as long as it returns false
13:35jandemtill: could you use EnvironmentPreparer maybe?
13:37tilljandem: maybe, let me read up on that
13:50tilljandem: EnvironmentPreparer looks like exactly what I need, thank you
13:51jandemtill: cool, np
14:16The_8472dherman, does the realm API or something related have a way to load scripts? not via eval but by URI. like <script> tags do for the web.
14:18nbpThe_8472: s = document.createElement(&quot;script&quot;); ; document.body.appendChild(s); ?
14:18The_8472nbp, in a realm, not in the main global
14:19nbpThe_8472: ok, I don&#39;t know what you mean by &quot;realm API&quot;.
14:21nbpThe_8472: would module be an answer for your question?
14:21nbpThe_8472: or you mean loading dynamically?
14:22The_8472loading dynamically. like realm.loadSync(&quot;blob:....&quot;)
14:23nbpThe_8472: then I have no answers for you, apart from JS engine having a load function.
14:23* dherman commuting bbl
14:28The_8472left a comment on
14:28crowbotIssue #16: Need some way to evaluate code as global code -
14:35nbpcrowbot: botsnack
14:35crowbotom nom nom
14:41sfinkwhat do I need to change in order to add a new call to addStubField in a CacheIR guard?
14:41tcampbellsfink: If it is an existing type, should automagically work
14:41sfinkwhen I just added one in, I got Assertion failure: aIndex < mLength for a *different* guard
14:42sfink(I added one to guardCompartment, and am getting the assertion under emitGuardIsString)
14:44tcampbellhmm.. that is odd
14:46tcampbellThere does seem to be a byte limit on stub fields
14:48sfinkwith my addition, I&#39;m at 24 out of 160
14:48sfinkthe whole patch is
14:53tcampbellsfink: you might need to deserialize the stub so cacheIR does not get confused
14:54tcampbell(not super sure)
14:54sfinkooh, that looks promising! thanks!
14:54tcampbellotherwise, jandem will certainly know
14:55tcampbellsfink: don&#39;t forget both baseline and ion
14:55sfinkyep, I did them both
14:55sfinkit worked! thank you very much
14:56sfinkand it totally makes sense why that would be needed. Well, now it does, anyway. ;-)
14:56sfinksadly, it didn&#39;t fix my actual bug...
14:56bbouviergosh, c++ is terrible
14:57sfinkjust wait until you see c++
14:57sfink(there are so many)
14:57bbouvieri&#39;m creating a non-copiable-but-movable struct, using it as a key in a HashMap, and it somehow ends up being not initialized when i get over a Range on the hashmap
14:59bbouvierdoes this bug sound familiar to anybody? i could really use some help
15:01sfinkit&#39;s uninitialized in the underlying HashMapEntry?
15:02sfinkis the Entry&#39;s address different between the time you inserted it and the time you grab it out of the Range?
15:03sfink(I don&#39;t know what might be going wrong, but I&#39;m thinking of how to debug it to get closer)
15:05bbouvieri use the Move operator when adding the key to the hash map, so i suspect the addresses would be different
15:06nbpbbouvier: Range expects no modification to happen to the hash map while you iterate with the Range
15:06sfinkI wanted to compare if it was the same immediately after insertion as it is when you iterate
15:07sfinkbut is it uninitialized right after it lands in the HashMap (after the Move)?
15:07bbouviersfink: checking
15:07bbouviernbp: right, i&#39;m not updating the map as i iterate over it with the Range
15:07sfinkI thought there was an assertion for that
15:09nbpbbouvier: The Key is not copiable, can you log when the key is moved out?
15:10nbpbbouvier: and use rr to print the stack where the logging happen.
15:10sfinkif the addresses are the same, I was going to suggest doing a hw watchpoint to see when it happens
15:10nbpbbouvier: or just break on the move constructor and print the stack.
15:10sfinkgood idea
15:19bbouvierhum, actually only one part of the Key is going nuts, one which contains a Vector
15:19bbouvierjust after it&#39;s inserted in the hashmap, it&#39;s correct, but on the next lookup, it&#39;s not
15:24djvjnaveed: ping
15:25djvjnaveed: I need to get VTune on windows. Pinging bas about this now.
15:27bbouviersfink, nbp: okay, so it happens i&#39;m stupid and was using a const& for the vector field, which is a bad idea, because C++ will silently use a reference to the stack memory in that case when you use the Move operator
15:27bbouviersfink, nbp: fwiw, i also share some of that stupidity with c++ which allows me doing this and doesn&#39;t warn or anything
15:27bbouvier(asan would have told me, probably)
15:28sfinkok, that makes sense
15:28sfinknot sure about the &quot;stupid&quot; part, though; you need to be pretty smart to be &quot;stupid&quot; in such a way :-)
15:39bbouviersfink, nbp: thanks for the help, btw :)
15:42naveeddjvj: talk to sstangl about this
15:52bzDo class declarations inhibit ion compilation?
15:53tillI think they do, yes
15:53tillbz: bug 1167472
15:53firebot NEW, Make classes work in the JITs.
15:54tcampbellbz: I plan to get to it this week/next
15:55djvjnaveed: already did. he&#39;s using VTune licenses for Linux. Doesn&#39;t know about windows licenses.
15:55djvjnaveed: Bas said he got his from ServiceNow. sstangl mentioned that vlad might have one
15:55bztill, tcampbell: Ah, thank you!
15:55djvjnaveed: asking him. if I can&#39;t find one, can I order it?
15:56* bz searched for &quot;class ion&quot; but didn&#39;t realize we have no baseline support either
15:57tcampbellhopefully it is straightforward
15:58naveeddjvj: i think it is super expensive. we have 5 linux licenses and it can remote debug windows
15:59naveeddjvj: find out hwo much it is
15:59djvjnaveed: $900 apparently
15:59djvjnaveed: I&#39;ll try to get a windows license, if not I&#39;ll follow the remote-debug route
15:59naveeddjvj: probably worth reclaiming licenses first
15:59djvjthat&#39;s what I mean..
16:01bztill: ping
16:01bztill: want me to steal ?
16:01firebotBug 1067942 ASSIGNED, &quot;TypeError: setting a property that has only a getter&quot; without mentioning file and property name
16:01tillbz: yes please!
16:01bztill: Will do
16:02tillbz: thank you!
16:02* till is really quite stupid for not landing that :(
16:04evilpiesfink: thanks
16:06bbouviersomebody else has a shell startup crash on cross-built 32 bits shells? (ubuntu 16.04 64 bits)
16:08bbouvier(happening in the PLT, like the vtune crash)
16:09bbouviercalling into BaseTimeDurationPlatformUtils::TicksFromMilliseconds
16:10bbouviernevermind, doing a full clobber build made it disappear...
16:14bztill: well, you did, but then you got backed out for test failures....
16:14bztill: So there&#39;s work to be done there. ;)
16:21sfinkdjvj: can you use a 30-day trial to get started?
16:38tillbz: clearly, yes :(
16:59djvjsfink: probably, looking into it.
18:12bztill: wow, this has totally bitrotted....
18:14bzReportGetterOnlyAssignment is dead code?
18:22* bz realizes this patch is both much simpler and much different now
18:24tillbz: probably jandem&#39;s error reporting refactoring?
18:25bzwell, and jorendorff&#39;s?
18:25bzAll in terms of ObjectOpResult now
18:26bzAnyway, building, will see whether this really fixes things as I expect
18:26bzAnd then it&#39;s off to trypush. ;)
19:31h4writersstangl: ping
19:32sstanglh4writer: pong
19:32sstangljust about to grab lunch
19:32h4writersstangl: quick question: Have you been able to look at bug 1342016
19:32firebot NEW, Regression on Jan 11th 2017 on misc benchmarks
19:32h4writerand have a good meal
19:33sstanglh4writer: briefly yeah, but I wasn&#39;t able to narrow it down. It only repros with parallel compilation, and if I attempt to extract iongraphs they look identical
19:35h4writersstangl: have you tried using a jitspew-enabled optimized build?
19:36sstanglnope, I&#39;ll give that a try.
19:36h4writersstangl: would be good to have it fixed. The regression landed in bug 53, which means we don&#39;t have that much time anymore
19:36sstanglOK, I&#39;ll prioritize it.
19:37h4writersstangl: thanks and bon apptit
20:29nbpjandem: The Netherlands second?
20:50shuWaldo: got a bug for the utf8 lexer?
20:50Waldoshu: I was just planning on using the existing single-byte parsing bug, don&#39;t remember its number
20:51shuWaldo: is it the one i filed about rewrite the frontend: lexer
20:52shuWaldo: in any case we should file the refactoring bugs separately and land them separately
20:52shuWaldo: i&#39;ll file the one for getting errors out of TokenStream, can you do one for the templatizing?
20:52Waldoshu: sure
21:47bzjandem: ping
22:06sstanglI just realized that a shell build directory is 2GB
22:06sstanglthat seems like a lot
22:12sstangl441MB of that is gdb-tests and jsapi-tests, which I don&#39;t need
22:14sstangl568MB for libjs_static.a; 178MB for
22:15sfinkmine is 2.0GB too. If I do a recursive strip --strip-debug, it drops to 564MB.
22:15sstanglbetter to not write bits I don&#39;t need in the first place. When debugging I don&#39;t want stripped executables.
22:16sfinkwe could default just gdb-tests and jsapi-tests to not save debug info
22:17sfink(or not default them, but provide per-directory options or something)
22:25sstanglfiled 1347729
23:41gkwsfink: bug 1347750
23:41firebot NEW, Having a striped js shell breaks fuzzing on releng js shells that we ship
23:41gkwsfink: I thought it was nice that releng js shells had symbols, so starting fuzzing on them, turns out it is a bug and it was fixed by stripping the exec
23:42gkwso we&#39;re back to the same old story of having no symbols
23:45ntimbz: ping
23:46ntimor smaug
23:46gkwsfink: thanks for the lang fix
23:47gkwlol @ zebra-striped js shells
23:47sfinkwhich shells do you use? Every push? Nightly?
23:48sfinkmaybe we could strip the debug info into a separate file
23:48* sfink suggests in bug
23:50gkwsfink: we had been using tinderbox-builds
23:50gkwsfink: so that was every-push I guess
23:51gkwsfink: thanks for helping out
16 Mar 2017
No messages
Last message: 8 days and 7 hours ago