153923 2016-02-05 11:34:18 -0800 REGRESSION (r193373): bmalloc: largeMax calculation is wrong on iOS 2016-02-09 18:50:20 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 151817 1 saam saam ap benjamin ddkilzer fpizlo ggaren gskachkov keith_miller mark.lam msaboff oliver sukolsak ysuzuki oldest_to_newest 1162443 0 saam 2016-02-05 11:34:18 -0800 largeMax + sizeof(LargeChunk) > 1MB which is bad. 1162451 1 270757 saam 2016-02-05 12:15:54 -0800 Created attachment 270757 patch 1162455 2 270757 mark.lam 2016-02-05 12:23:27 -0800 Comment on attachment 270757 patch View in context: https://bugs.webkit.org/attachment.cgi?id=270757&action=review r=me with fixes. > Source/bmalloc/ChangeLog:11 > + space to actually allocate inside the LargeChunk. This made > + it so that we tried to allocate a large object for something > + that really should be extra large. Did you mean "This made it so that we will allocate a large object for something that should really be extra large."? > Source/bmalloc/ChangeLog:14 > + which meant that when we would grow() and allocate an Remove "an"? > Source/bmalloc/ChangeLog:16 > + a new LargeObject, we wouldn't actually be able to fit > + in the object we were growing for into the new slot. Did you mean "wouldn't actually be able to fit the object"? > Source/bmalloc/bmalloc/LargeChunk.h:82 > +static_assert(largeChunkMetadataSize + largeMax <= largeChunkSize, "We will think we can accomidate larger objects than we can in reality if this condition does not hold"); typo: accomidate ==> accomodate. 1162463 3 270757 msaboff 2016-02-05 12:43:01 -0800 Comment on attachment 270757 patch View in context: https://bugs.webkit.org/attachment.cgi?id=270757&action=review > Source/bmalloc/bmalloc/Sizes.h:79 > +#if BPLATFORM(IOS) > + static const size_t largeChunkMetadataSize = 16 * kB; > +#else > + static const size_t largeChunkMetadataSize = 4 * kB; > +#endif Can you change this to just be vmPageSize if that what the math should be? 1162470 4 saam 2016-02-05 13:10:20 -0800 (In reply to comment #3) > Comment on attachment 270757 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270757&action=review > > > Source/bmalloc/bmalloc/Sizes.h:79 > > +#if BPLATFORM(IOS) > > + static const size_t largeChunkMetadataSize = 16 * kB; > > +#else > > + static const size_t largeChunkMetadataSize = 4 * kB; > > +#endif > > Can you change this to just be vmPageSize if that what the math should be? I was thinking that it lines up nicely, but it's not required that they're the same, even though they're currently the same. It just needs to be aligned on page boundaries. I think it makes sense to keep the math as is as a better descriptor of intention. 1162471 5 saam 2016-02-05 13:10:59 -0800 We could add a static assert that the object is divisible by the system's page size. 1162472 6 270757 ggaren 2016-02-05 13:14:24 -0800 Comment on attachment 270757 patch View in context: https://bugs.webkit.org/attachment.cgi?id=270757&action=review > Source/bmalloc/bmalloc/LargeChunk.h:81 > +static_assert(largeChunkMetadataSize == sizeof(LargeChunk), "largeChunkMetadataSize should be the number as sizeof(LargeChunk) or our math in Size.h is wrong"); Sizes.h You can be more specific and mention largeMax >> Source/bmalloc/bmalloc/LargeChunk.h:82 >> +static_assert(largeChunkMetadataSize + largeMax <= largeChunkSize, "We will think we can accomidate larger objects than we can in reality if this condition does not hold"); > > typo: accomidate ==> accomodate. I would remove "if this condition does not hold" -- all assertions are like that -- you only see the message because the condition didn't hold. 1162483 7 saam 2016-02-05 13:34:54 -0800 landed in: http://trac.webkit.org/changeset/196186 1163367 8 ddkilzer 2016-02-09 18:49:39 -0800 <rdar://problem/18620635> 1163368 9 ddkilzer 2016-02-09 18:50:20 -0800 Per internal discussion, this was a regression from the fix for Bug 151817. Bug 151817: bmalloc: extra large allocations could be more efficient <https://bugs.webkit.org/show_bug.cgi?id=151817> <http://trac.webkit.org/changeset/193373> 270757 2016-02-05 12:15:54 -0800 2016-02-05 12:23:27 -0800 patch c-backup.diff text/plain 3706 saam SW5kZXg6IFNvdXJjZS9ibWFsbG9jL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvYm1h bGxvYy9DaGFuZ2VMb2cJKHJldmlzaW9uIDE5NjE3OSkKKysrIFNvdXJjZS9ibWFsbG9jL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDMwIEBACisyMDE2LTAyLTA1ICBTYWFtIGJh cmF0aSAgPHNiYXJhdGlAYXBwbGUuY29tPgorCisgICAgICAgIGJtYWxsb2M6IGxhcmdlTWF4IGNh bGN1bGF0aW9uIGlzIHdyb25nIG9uIGlPUworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MTUzOTIzCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgT3VyIG51bWJlciBmb3IgbGFyZ2VNYXggd2FzIGxhcmdlciB0aGFu IHdoYXQgd2UgaGFkCisgICAgICAgIHNwYWNlIHRvIGFjdHVhbGx5IGFsbG9jYXRlIGluc2lkZSB0 aGUgTGFyZ2VDaHVuay4gVGhpcyBtYWRlIAorICAgICAgICBpdCBzbyB0aGF0IHdlIHRyaWVkIHRv IGFsbG9jYXRlIGEgbGFyZ2Ugb2JqZWN0IGZvciBzb21ldGhpbmcKKyAgICAgICAgdGhhdCByZWFs bHkgc2hvdWxkIGJlIGV4dHJhIGxhcmdlLgorICAgICAgICBQcmV2aW91c2x5LAorICAgICAgICBs YXJnZU1heCArIHNpemVvZihMYXJnZUNodW5rKSA+IDFNQgorICAgICAgICB3aGljaCBtZWFudCB0 aGF0IHdoZW4gd2Ugd291bGQgZ3JvdygpIGFuZCBhbGxvY2F0ZSBhbgorICAgICAgICBhIG5ldyBM YXJnZU9iamVjdCwgd2Ugd291bGRuJ3QgYWN0dWFsbHkgYmUgYWJsZSB0byBmaXQKKyAgICAgICAg aW4gdGhlIG9iamVjdCB3ZSB3ZXJlIGdyb3dpbmcgZm9yIGludG8gdGhlIG5ldyBzbG90LgorICAg ICAgICBUaGlzIHdvdWxkIG1hbmlmZXN0IHdoZW4gd2UgaGFkIGFuIGFsbG9jYXRpb24gc2l6ZSBp biB0aGUgcmFuZ2U6CisgICAgICAgIDFNQiAtIHNpemVvZihMYXJnZUNodW5rKSA8IGFsbG9jYXRp b24gc2l6ZSA8IGxhcmdlTWF4CisKKyAgICAgICAgV2UgZml4IHRoaXMgYnVnIGJ5IGJlaW5nIHBy ZWNpc2UgaW4gb3VyIGNhbGN1bGF0aW9uIG9mIGxhcmdlTWF4CisgICAgICAgIGluc3RlYWQgb2Yg anVzdCBhc3N1bWluZyBsYXJnZUNodW5rU2l6ZSAqIDk5LzEwMCBpcyBlbm91Z2gKKyAgICAgICAg c3BhY2UgZm9yIHRoZSBtZXRhZGF0YS4KKworICAgICAgICAqIGJtYWxsb2MvTGFyZ2VDaHVuay5o OgorICAgICAgICAoYm1hbGxvYzo6TGFyZ2VDaHVuazo6Z2V0KToKKyAgICAgICAgKiBibWFsbG9j L1NpemVzLmg6CisKIDIwMTYtMDEtMzEgIERhbiBCZXJuc3RlaW4gIDxtaXR6QGFwcGxlLmNvbT4K IAogICAgICAgICBbQ29jb2FdIFJlbW92ZSB1bnVzZWQgZGVmaW5pdGlvbiBvZiBIQVZFX0hFQURF Ul9ERVRFQ1RJT05fSApJbmRleDogU291cmNlL2JtYWxsb2MvYm1hbGxvYy9MYXJnZUNodW5rLmgK PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PQotLS0gU291cmNlL2JtYWxsb2MvYm1hbGxvYy9MYXJnZUNodW5rLmgJKHJldmlz aW9uIDE5NjE3NCkKKysrIFNvdXJjZS9ibWFsbG9jL2JtYWxsb2MvTGFyZ2VDaHVuay5oCSh3b3Jr aW5nIGNvcHkpCkBAIC03OCw2ICs3OCw5IEBAIHByaXZhdGU6CiAjZW5kaWYKIH07CiAKK3N0YXRp Y19hc3NlcnQobGFyZ2VDaHVua01ldGFkYXRhU2l6ZSA9PSBzaXplb2YoTGFyZ2VDaHVuayksICJs YXJnZUNodW5rTWV0YWRhdGFTaXplIHNob3VsZCBiZSB0aGUgbnVtYmVyIGFzIHNpemVvZihMYXJn ZUNodW5rKSBvciBvdXIgbWF0aCBpbiBTaXplLmggaXMgd3JvbmciKTsKK3N0YXRpY19hc3NlcnQo bGFyZ2VDaHVua01ldGFkYXRhU2l6ZSArIGxhcmdlTWF4IDw9IGxhcmdlQ2h1bmtTaXplLCAiV2Ug d2lsbCB0aGluayB3ZSBjYW4gYWNjb21pZGF0ZSBsYXJnZXIgb2JqZWN0cyB0aGFuIHdlIGNhbiBp biByZWFsaXR5IGlmIHRoaXMgY29uZGl0aW9uIGRvZXMgbm90IGhvbGQiKTsKKwogaW5saW5lIExh cmdlQ2h1bmsqIExhcmdlQ2h1bms6OmdldCh2b2lkKiBvYmplY3QpCiB7CiAgICAgQkFTU0VSVCgh aXNTbWFsbE9yTWVkaXVtKG9iamVjdCkpOwpJbmRleDogU291cmNlL2JtYWxsb2MvYm1hbGxvYy9T aXplcy5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9ibWFsbG9jL2JtYWxsb2MvU2l6ZXMuaAkocmV2 aXNpb24gMTk2MTc0KQorKysgU291cmNlL2JtYWxsb2MvYm1hbGxvYy9TaXplcy5oCSh3b3JraW5n IGNvcHkpCkBAIC03MiwxMSArNzIsMTYgQEAgbmFtZXNwYWNlIFNpemVzIHsKICAgICBzdGF0aWMg Y29uc3Qgc2l6ZV90IG1lZGl1bUNodW5rTWFzayA9IH4obWVkaXVtQ2h1bmtTaXplIC0gMXVsKTsK IAogICAgIHN0YXRpYyBjb25zdCBzaXplX3QgbGFyZ2VDaHVua1NpemUgPSBzdXBlckNodW5rU2l6 ZSAvIDI7CisjaWYgQlBMQVRGT1JNKElPUykKKyAgICBzdGF0aWMgY29uc3Qgc2l6ZV90IGxhcmdl Q2h1bmtNZXRhZGF0YVNpemUgPSAxNiAqIGtCOworI2Vsc2UKKyAgICBzdGF0aWMgY29uc3Qgc2l6 ZV90IGxhcmdlQ2h1bmtNZXRhZGF0YVNpemUgPSA0ICoga0I7CisjZW5kaWYKICAgICBzdGF0aWMg Y29uc3Qgc2l6ZV90IGxhcmdlQ2h1bmtPZmZzZXQgPSAwOwogICAgIHN0YXRpYyBjb25zdCBzaXpl X3QgbGFyZ2VDaHVua01hc2sgPSB+KGxhcmdlQ2h1bmtTaXplIC0gMXVsKTsKIAogICAgIHN0YXRp YyBjb25zdCBzaXplX3QgbGFyZ2VBbGlnbm1lbnQgPSA2NDsKLSAgICBzdGF0aWMgY29uc3Qgc2l6 ZV90IGxhcmdlTWF4ID0gbGFyZ2VDaHVua1NpemUgKiA5OSAvIDEwMDsgLy8gUGxlbnR5IG9mIHJv b20gZm9yIG1ldGFkYXRhLgorICAgIHN0YXRpYyBjb25zdCBzaXplX3QgbGFyZ2VNYXggPSBsYXJn ZUNodW5rU2l6ZSAtIGxhcmdlQ2h1bmtNZXRhZGF0YVNpemU7CiAgICAgc3RhdGljIGNvbnN0IHNp emVfdCBsYXJnZU1pbiA9IG1lZGl1bU1heDsKICAgICAKICAgICBzdGF0aWMgY29uc3Qgc2l6ZV90 IHhMYXJnZUFsaWdubWVudCA9IHZtUGFnZVNpemU7CkluZGV4OiBTb3VyY2UvYm1hbGxvYy9ibWFs bG9jL1ZNSGVhcC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL2JtYWxsb2MvYm1hbGxvYy9WTUhl YXAuY3BwCShyZXZpc2lvbiAxOTYxNzQpCisrKyBTb3VyY2UvYm1hbGxvYy9ibWFsbG9jL1ZNSGVh cC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTUzLDcgKzUzLDkgQEAgdm9pZCBWTUhlYXA6Omdyb3co KQogICAgICAgICBtX21lZGl1bVBhZ2VzLnB1c2goaXQpOwogCiAgICAgTGFyZ2VDaHVuayogbGFy Z2VDaHVuayA9IHN1cGVyQ2h1bmstPmxhcmdlQ2h1bmsoKTsKLSAgICBtX2xhcmdlT2JqZWN0cy5p bnNlcnQoTGFyZ2VPYmplY3QoTGFyZ2VPYmplY3Q6OmluaXQobGFyZ2VDaHVuaykuYmVnaW4oKSkp OworICAgIExhcmdlT2JqZWN0IHJlc3VsdChMYXJnZU9iamVjdDo6aW5pdChsYXJnZUNodW5rKS5i ZWdpbigpKTsKKyAgICBCQVNTRVJUKHJlc3VsdC5zaXplKCkgPT0gbGFyZ2VNYXgpOworICAgIG1f bGFyZ2VPYmplY3RzLmluc2VydChyZXN1bHQpOwogfQogCiB9IC8vIG5hbWVzcGFjZSBibWFsbG9j Cg==