[HN Gopher] SlowDownAndWasteMemory()
___________________________________________________________________
SlowDownAndWasteMemory()
Author : lopkeny12ko
Score : 184 points
Date : 2023-07-03 16:57 UTC (6 hours ago)
(HTM) web link (github.com)
(TXT) w3m dump (github.com)
| fwlr wrote:
| Big fan of these "sarcastically named" functions, there are a lot
| of good reasons to use them (mainly, every time you go to use it
| you'll think about whether it's really needed) and not many
| reasons to not use it (naming it "fastConvert" doesn't make it
| any faster than naming it "slowConvert").
| 8n4vidtmkvmk wrote:
| I'd assume fastConvert is less accurate. If it was both fast
| and perfect it'd just be 'convert'
| BoppreH wrote:
| Or it only accepts certain inputs, or has an expensive first
| invocation but amortized cost, or is not bug-compatible with
| the old `convert`, or it requires a large cache, or it's only
| fast single-threaded, or it was the experimental replacement
| whose name stuck around, or...
| forty wrote:
| Fast doesn't always mean good. For example in cryptography a
| fonction named fastCompare could be a non constant time
| comparison which is faster but not safe for cryptography :D
| (though you'd probably want to name it unsafeButFastCompare I
| guess ^^)
| kstrauser wrote:
| Some of our internal tools have command line options along the
| lines of: --yolo Yes, I really want to
| overwrite the production database and get us all fired.
|
| where "--yolo" is our standard flag for such things. When I
| find myself about to type that, I go back and double, triple,
| quadruple check that I really intend to do the thing that I'm
| asking it to do.
| vidarh wrote:
| MySQL of course has (had? Not used it for an eternity) the
| --i-am-a-dummy flag, which felt like it should be reversed
| (it prevents update/delete without an explicit where clause,
| but would be more appropriate if you had to use it to allow
| them).
| adhesive_wombat wrote:
| Shades of hdparm's --please-destroy-my-drive
| kstrauser wrote:
| I applaud that one, too.
| nurple wrote:
| Love it! Ceph has --yes-i-really-mean-it for dangerous fs
| changes, and --yes-i-really-really-mean-it for pool rm. The
| annoying-to-type property is pretty effective.
| Sohcahtoa82 wrote:
| Reminds me of runWithScissors.
|
| https://android.googlesource.com/platform/frameworks/base/+/...
| Jarred wrote:
| You might also like JSGlobalObject::haveABadTime()
|
| https://github.com/WebKit/WebKit/blob/b305f64a67e288b714a84f...
| // Consider the following objects and prototype chains:
| // O (of global G1) -> A (of global G1) // B (of
| global G2) where G2 has a bad time // // If we
| set B as the prototype of A, G1 will need to have a bad time.
| // See comments in Structure::mayInterceptIndexedAccesses() for
| why. // // Now, consider the following objects
| and prototype chains: // O1 (of global G1) -> A1 (of
| global G1) -> B1 (of global G2) // O2 (of global G2)
| -> A2 (of global G2) // B2 (of global G3) where G3 has
| a bad time. // // G1 and G2 does not have a bad
| time, but G3 already has a bad time. // If we set B2 as
| the prototype of A2, then G2 needs to have a bad time. //
| Note that by induction, G1 also now needs to have a bad time
| because of // O1 -> A1 -> B1. // // We
| describe this as global G1 being affected by global G2, and G2 by
| G3. // Similarly, we say that G1 is dependent on G2, and
| G2 on G3. // Hence, when G3 has a bad time, we need to
| ensure that all globals that // are transitively
| dependent on it also have a bad time (G2 and G1 in this
| // example). // // Apart from clearing the VM
| structure cache above, there are 2 more things // that we
| have to do when globals have a bad time: // 1. For each
| affected global: // a. Fire its HaveABadTime
| watchpoint. // b. Convert all of its array structures
| to SlowPutArrayStorage. // 2. Make sure that all affected
| objects switch to the slow kind of // indexed
| storage. An object is considered to be affected if it has
| // indexed storage and has a prototype object which may have
| indexed // accessors. If the prototype object belongs
| to a global having a bad // time, then the prototype
| object is considered to possibly have indexed //
| accessors. See comments in
| Structure::mayInterceptIndexedAccesses() // for
| details. // // Note: step 1 must be completed
| before step 2 because step 2 relies on // the
| HaveABadTime watchpoint having already been fired on all affected
| // globals. // // In the common case, only this
| global will start having a bad time here, // and no other
| globals are affected by it. So, we first proceed on this
| assumption // with a simpler
| ObjectsWithBrokenIndexingFinder scan to find heap objects
| // affected by this global that need to be converted to
| SlowPutArrayStorage. // We'll also have the finder check
| for the presence of other global objects // depending on
| this one. // // If we do discover other globals
| depending on this one, we'll abort this // first
| ObjectsWithBrokenIndexingFinder scan because it will be
| insufficient // to find all affected objects that need to
| be converted to SlowPutArrayStorage. // It also does not
| make dependent globals have a bad time. Instead, we'll //
| take a more comprehensive approach of first creating a dependency
| graph // between globals, and then using that graph to
| determine all affected // globals and objects. With that,
| we can make all affected globals have a // bad time, and
| convert all affected objects to SlowPutArrayStorage.
| Waterluvian wrote:
| isResizableOrGrowableSharedTypedArrayIncludingDataView
|
| One of those irrational times of my year is when I inevitably hit
| something that really deserves a good descriptor but there just
| isn't a good word or two for it. So I end up with one of these
| monsters.
|
| It's not a real problem, but it's a moment that doesn't bring me
| joy.
| exabrial wrote:
| [flagged]
| vGPU wrote:
| [flagged]
| [deleted]
| fuu_dev wrote:
| got attention from this tweet:
| https://twitter.com/jarredsumner/status/1675732777873063936?...
|
| Quote for people without account or rate limited:
|
| "When you do uint8array.buffer in JSC the first time, it calls
| slowDownAndWasteMemory()" -- links to the github
|
| The tweet has ~60k views, and 24 retweets.
| olliej wrote:
| alas that's not accessible anymore
| RoyGBivCap wrote:
| [flagged]
| lwansbrough wrote:
| GP is referring to the tweet itself, which is behind an
| login screen if you don't have a Twitter account.
| RoyGBivCap wrote:
| [flagged]
| Forbo wrote:
| Your definition of accessible reminds me of Cave Johnson.
|
| "Bean counters said I couldn't fire a man for being in a
| wheelchair. Did it anyway. Ramps are expensive."
| olliej wrote:
| "Something went wrong. Try reloading."
|
| > Y'all's hatred of Musk is driving you to lie.
|
| My god, my hatred for Musk has caused twitter to go down.
| Wow, I didn't know I had such power. JFC.
| KK8xuwHEYQ wrote:
| [dead]
| maury91 wrote:
| But he is not lying, it is inaccessible for some people,
| myself included, I get the same error:
|
| https://i.imgur.com/44KNIlg.jpg
| AMC11 wrote:
| For what it's worth, I can't access it either, so there
| are probably many of us.
| olliej wrote:
| Yeah, but based on his musk comment I'm going to assume
| anything we say isn't relevant.
|
| He's accusing people of lying because they hate musk,
| when they're saying content is not accessible on a site
| that is currently documented to be suffering basic
| response problems. It's the kind of reactive BS you get
| from idiots who aren't interested in facts or reality so
| _shrug_.
| Faceted4843 wrote:
| [dead]
| Shared404 wrote:
| "Something went wrong, try reloading"
|
| And even then, I've never had a Twitter account, so not
| sure I'd be able to see it now anyways.
|
| Sure seems inaccessible to me.
| [deleted]
| vGPU wrote:
| To try to prevent others from joining this argument: twitter
| is currently not allowing you to read tweets without being
| logged in. Hence, it can be both available and not available.
| olliej wrote:
| If it cannot be read without logging in, then it is
| inaccessible. That's a choice twitter gets to make, but to
| claim tweets are accessible because some people can read
| them is no different from posting links to a private
| facebook group and saying "you just need to be a member of
| that group".
|
| That said, I was saying it is inaccessible because I am
| getting error messages trying to load it.
| dylan604 wrote:
| >Hence, it can be both available and not available.
|
| So Musk has given us the real life Schrodinger's cat?
| lopkeny12ko wrote:
| [flagged]
| Percent1361 wrote:
| [dead]
| tomjakubowski wrote:
| almost certainly an innocent mistake, not "lying"
| tambourine_man wrote:
| That's Twitter for you, these days.
| traes wrote:
| Wouldn't it make more sense to directly link to the line in the
| title?
|
| https://github.com/WebKit/WebKit/blob/ab10a90523e06df54bbb8a...
| mrpf1ster wrote:
| I believe permalinks are removed when submitting
| lucb1e wrote:
| (Note that this is not called a permalink but a fragment
| identifier
| https://www.w3.org/Addressing/URL/4_2_Fragments.html or, more
| memorably, the hash part of the URL
| https://developer.mozilla.org/en-US/docs/Web/API/URL/hash.
| Permalinks can be entirely different URLs, like a DOI link
| that is on a different domain and protocol altogether, but
| that will keep referring to this content
| https://en.wikipedia.org/wiki/Permalink)
| ryanisnan wrote:
| Thanks! I referred to them as anchors.
| dang wrote:
| Ok, I've put it (back?) in there.
| nektro wrote:
| relevant code on line 269
| Alifatisk wrote:
| Namespace WTF
|
| https://github.com/WebKit/WebKit/blob/ab10a90523e06df54bbb8a...
| Tade0 wrote:
| Explicit is better than implicit.
| u_name wrote:
| My personal favorite:
| ResizableNonSharedAutoLengthWastefulTypedArray
| the_mitsuhiko wrote:
| It's short for Web Template Framework.
| dmitshur wrote:
| "WTF is Web Template Framework"
| xyst wrote:
| Definitely ^_^
| [deleted]
| pizlonator wrote:
| But it's pronounced What The Fuck.
| yurishimo wrote:
| Good to see you in this thread being a good sport!
|
| Thanks for your contributions to Webkit :)
| jakeogh wrote:
| I like https://surf.suckless.org (which uses WebKit) but it seems
| like every time I use it and leave a window open for awhile, that
| process (WebkitWebProcess) will end up pegging a core for no
| obvious reason. It's not site dependent, and I think it's when JS
| is enabled.
| lloydatkinson wrote:
| Why does it appear to have an awful default font?
| Alifatisk wrote:
| Link leads to 404
|
| Do you mean https://surf.suckless.org ?
| jakeogh wrote:
| Thanks:)
| pizlonator wrote:
| 100% probability that the behavior you observe has 0% to do
| with this function.
| jakeogh wrote:
| Oh I agree. I suppose I should read
| https://www.webkit.org/debugging-webkit/. It's a problem I
| cant trivially reproduce on command, so (reading your comment
| above) I wonder how you would approach it.
| londons_explore wrote:
| I would just run "sudo perf record" to do a systemwide
| performance trace, followed by "sudo perf report" and
| you'll find the function name where it's spending all the
| CPU cycles.
|
| Another option is to just "sudo gdb -p 12345" to attach to
| the process, then "thread apply all bt" to get stack traces
| of each thread, and find whichever isn't sat in " _wait_ ".
| That has the benefit you can inspect local variables in
| every frame etc (for opensource stuff, symbols are usually
| available automatically via debuginfod).
|
| Chromium/blink has much better built in performance tracing
| things if you can reproduce there - see chrome://tracing/
| for tracing internal browser components, or the Performance
| tab in devtools if you think it is javascript gobbling all
| the CPU time.
| anthk wrote:
| All webkitgtk4 browsers run the same, from surf to luakit to
| vimb to midori.
| asveikau wrote:
| Some of the identifier names are kind of casual here. I was
| amused by the class called _DeferGCForAWhile_. I 'm guessing just
| from reading that it's a typical RAII pattern so the "while" that
| will be deferred may be the scope of the object, i.e. until its
| destructor is called.
| kazinator wrote:
| Love that printInternal method. If the caller wants to map the
| enum to the string, make them know about a PrintStream that they
| have to instantiate. (And possibly write their own derivation, if
| there isn't one that captures output into a string.)
| afavour wrote:
| // We play this game because we want this to be callable even
| from places that // don't have access to CallFrame\* or
| the VM, and we only allocate so little // memory here
| that it's not necessary to trigger a GC - just accounting what
| // we have done is good enough. The sort of bizarre exception to
| the "allocating // little memory" is when we transfer a
| backing buffer into the C heap; this // will temporarily
| get counted towards heap footprint (incorrectly, in the case
| // of adopting an oversize typed array) but we don't GC here
| anyway. That's // almost certainly fine. The worst case
| is if you created a ton of fast typed // arrays, and did
| nothing but caused all of them to slow down and waste memory.
| // In that case, your memory footprint will double before the GC
| realizes what's // up. But if you do *anything* to
| trigger a GC watermark check, it will know // that you
| *had* done those allocations and it will GC appropriately.
|
| Kind of interesting, really. The method is clearly somewhat
| sarcastically named but usefully so: they know what they're doing
| is not optimal but still do it anyway for the reasons outlined.
| It's not like this is laziness or nefariousness.
| pizlonator wrote:
| That comment is not the reason for the function's existence,
| but the details of how the function interacts with GC.
|
| The reason for the function's existence is that it allows typed
| arrays to dynamically switch between a fast/compact
| representation for the case that the JSVM owns the data, and a
| slightly slower and slightly less compact version for when the
| JSVM allows native code to share ownership of the data.
|
| This function, slowDownAndWasteMemory, switches to the less
| efficient version that allows aliasing with native code.
|
| Of course the name is sarcastic. The actual effect of having
| this is that JSC can handle both the owned case and the aliased
| case, and get a small will if you're in the owned case while
| being able to switch to the aliased case at any time. Since
| there's no way to support the aliased case without some
| slightly higher memory/time usage, we are sort of literally
| slowing down and wasting memory when we go aliased.
|
| Source: I'm pretty sure I wrote most of this code.
| kentonv wrote:
| Out of curiosity, what makes the aliased version less
| compact? Naively I'd think there's not much room to optimize
| the representation of a byte array.
| pizlonator wrote:
| Native code wants to think it's a ref counted buffer that
| could be accessed from any thread including ones that don't
| participate in GC. So it's gotta be allocated in the malloc
| heap and it's gotta have a ref count.
|
| JS code wants the buffer to be garbage collected. If native
| code doesn't ref it (count is zero) and it's not reachable
| via JS heap then it's gotta die. So, it needs a GC cell and
| associated machinery (JSC object headers, mark bits,
| possibly other stuff).
|
| So, the reason why the aliased case is not as awesome as
| the owned case is that you need double the things. You need
| some malloc memory and you need a GC cell. You need a ref
| count and you need a GC header and some mark bits.
|
| Having both malloc/RC and cell/GC overhead isn't like the
| end of the world or anything, but it means that allocations
| are slower and waste more memory than if it was either just
| a buffer owned by native code or just an object in the GC
| heap.
| qingcharles wrote:
| This is classic HN.
|
| "Look at this crazy, obscure code everybody!"
|
| * _ORIGINAL DEVELOPER APPEARS*_
| londons_explore wrote:
| Indeed you did, 10 years ago. This is the (mega!) commit:
|
| https://github.com/WebKit/WebKit/commit/93a48aa964f25ce8ec9f.
| ..
|
| I often wonder how much more productive I could be if I
| didn't need to split changes like this into 50 small commits,
| where every intermediate commit is working and has passing
| tests...
| kccqzy wrote:
| I find this to be more of a reflection of how well aligned
| you and the code reviewer are. When both of you know the
| code base inside out and trust each other, it's fine. But
| sometimes the author is a noob and the code reviewer has to
| review every little detail, much like an examiner looking
| at a student's paper. Or sometimes they might have a
| personal beef.
| pizlonator wrote:
| Yeah sometimes you just have to write a megapatch. I'd be
| very sad if I had to work on a project that didn't allow
| that.
| bpye wrote:
| I don't know what my largest commit has been, but I do
| find that merge conflicts end up being a real pain when
| you're touching so many files...
| AlotOfReading wrote:
| Mine was a very memorable ~2900 files changed.
|
| We missed an OS update cycle and had to consume all the
| changes in one giant patch. But upstream had migrated
| from Python 2 to 3 in-between and rewrote half their
| build scripts to accommodate. Each of those changes
| needed to be reviewed manually because we had modified
| things to support reproducible builds and the resulting
| merge conflict was monstrous. I contributed maybe 2300 of
| those before my sanity failed and called for help.
|
| Now we have a whole team to do that job more regularly.
| phatskat wrote:
| I understand the desire to make larger patches, but how
| do you effectively manage them in the review process? For
| super large commits in the past, I've had other engineers
| hop on a screen share to review the diff together and
| answer questions, but it feels inefficient.
| IshKebab wrote:
| You just don't review it as well. It's not the end of the
| world.
| jvanderbot wrote:
| You make sure that more than one person is aware of the
| changes and design, and have all people frequently
| discuss the progress before the mega patch hits PR.
|
| Then the mega patch PR is " one last look ", and not
| thousands of changes storming the gates of your sanity.
| 8n4vidtmkvmk wrote:
| Y'all are very thorough. Just make sure the function has
| tests and won't blow up prod. You needn't waste forever
| reviewing. It's code. It can be iterated upon.
| xvector wrote:
| This kind of mentality is absurdly inappropriate when
| working on something as security sensitive as a browser's
| JS engine.
| olliej wrote:
| No. In any actually complex piece of code even adding
| significant amounts testing over new code is not going to
| cover every possible code path.
|
| It's also immensely difficult to write tests to find
| errors in code you're writing - most patches I see with
| "extensive tests" test all the correct code paths, and
| _maybe_ a few error cases. It's a very easy trap to get
| yourself into.
|
| The purpose of review is not to catch obvious issues -
| though it hopefully will - but the subtle ones.
| londons_explore wrote:
| Indeed. I always push my teams into not hunting for
| spelling mistakes. Instead look for things that will be
| hard to fix in the future ('did you realise that this
| bootloader config stops us doing field updates??') or
| where the overall direction is wrong ('Not sure about
| os.system('grep') for this XML - why can't we just use
| the xml.etree here?').
| pdpi wrote:
| In this particular case, we're talking some tricky code
| around array handling in the JS implementation of a major
| browser. It's pretty much the archetypical location for a
| VM escape bug. That's most definitely not something you
| want to be cavalier about.
|
| In general, the large, sprawling diffs we're talking
| about here, the sort that actually justify commits of
| this size, are almost always going to also be the sort
| that justify more closer scrutiny at review time.
___________________________________________________________________
(page generated 2023-07-03 23:00 UTC)