92819 2012-07-31 18:22:09 -0700 MarkedAllocator::tryAllocateHelper() should sweep another block if it can't sweep a Structure block 2012-08-01 11:55:23 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 mhahnenberg mhahnenberg fpizlo ggaren oldest_to_newest 683722 0 mhahnenberg 2012-07-31 18:22:09 -0700 If we are forced to allocate a new block for Structures because we are unable to safely sweep our pre-existing Structure blocks, we should sweep another random block so that we can start sweeping Structure blocks sooner. 683725 1 155698 mhahnenberg 2012-07-31 18:27:27 -0700 Created attachment 155698 Patch 683803 2 155698 ggaren 2012-07-31 20:08:33 -0700 Comment on attachment 155698 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155698&action=review Looks good but I think I see a bug. > Source/JavaScriptCore/heap/IncrementalSweeper.cpp:92 > + while (m_currentBlockToSweepIndex < m_blocksToSweep.size()) { I don't think you want this loop here, do you? The goal of this function is to sweep just one. > Source/JavaScriptCore/heap/MarkedAllocator.cpp:38 > + m_heap->sweeper()->sweepNextBlock(); This could use a "why" comment. 683804 3 mhahnenberg 2012-07-31 20:10:33 -0700 (In reply to comment #2) > (From update of attachment 155698 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155698&action=review > > Looks good but I think I see a bug. > > > Source/JavaScriptCore/heap/IncrementalSweeper.cpp:92 > > + while (m_currentBlockToSweepIndex < m_blocksToSweep.size()) { > > I don't think you want this loop here, do you? The goal of this function is to sweep just one. The loop needs to be there in case the next block doesn't need sweeping (i.e. block->needsSweeping() == false). > This could use a "why" comment. Will do. 683807 4 mhahnenberg 2012-07-31 20:16:09 -0700 > The loop needs to be there in case the next block doesn't need sweeping (i.e. block->needsSweeping() == false). That being said, I'll come up with a better way to write it. 683825 5 155698 ggaren 2012-07-31 20:40:33 -0700 Comment on attachment 155698 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155698&action=review >>> Source/JavaScriptCore/heap/IncrementalSweeper.cpp:92 >>> + while (m_currentBlockToSweepIndex < m_blocksToSweep.size()) { >> >> I don't think you want this loop here, do you? The goal of this function is to sweep just one. > > The loop needs to be there in case the next block doesn't need sweeping (i.e. block->needsSweeping() == false). OK, I see where you're going with that. I still think it would be simpler (and slightly faster) to remove the extra loop. I see sweeping a block with no destructors as just a special fast case, and not a reason to keep sweeping. 684343 6 155698 ggaren 2012-08-01 11:13:45 -0700 Comment on attachment 155698 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155698&action=review Please add the "why" comment before landing. >>>> Source/JavaScriptCore/heap/IncrementalSweeper.cpp:92 >>>> + while (m_currentBlockToSweepIndex < m_blocksToSweep.size()) { >>> >>> I don't think you want this loop here, do you? The goal of this function is to sweep just one. >> >> The loop needs to be there in case the next block doesn't need sweeping (i.e. block->needsSweeping() == false). > > OK, I see where you're going with that. I still think it would be simpler (and slightly faster) to remove the extra loop. I see sweeping a block with no destructors as just a special fast case, and not a reason to keep sweeping. Eh, you convinced me. Let's keep it this way to keep things simple. 684413 7 mhahnenberg 2012-08-01 11:55:23 -0700 Committed r124352: <http://trac.webkit.org/changeset/124352> 155698 2012-07-31 18:27:27 -0700 2012-08-01 11:13:45 -0700 Patch bug-92819-20120731182710.patch text/plain 4016 mhahnenberg SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTI0Mjc4KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI0IEBA CisyMDEyLTA3LTMxICBNYXJrIEhhaG5lbmJlcmcgIDxtaGFobmVuYmVyZ0BhcHBsZS5jb20+CisK KyAgICAgICAgTWFya2VkQmxvY2s6OnN3ZWVwKCkgc2hvdWxkIHN3ZWVwIGFub3RoZXIgYmxvY2sg aWYgaXQgY2FuJ3Qgc3dlZXAgYSBTdHJ1Y3R1cmUgYmxvY2sKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTkyODE5CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSWYgd2UgYXJlIGZvcmNlZCB0byBhbGxvY2F0ZSBh IG5ldyBibG9jayBmb3IgU3RydWN0dXJlcyBiZWNhdXNlIHdlIGFyZSB1bmFibGUgdG8gc2FmZWx5 IAorICAgICAgICBzd2VlcCBvdXIgcHJlLWV4aXN0aW5nIFN0cnVjdHVyZSBibG9ja3MsIHdlIHNo b3VsZCBzd2VlcCBhbm90aGVyIHJhbmRvbSBibG9jayBzbyB0aGF0IHdlIAorICAgICAgICBjYW4g c3RhcnQgc3dlZXBpbmcgU3RydWN0dXJlIGJsb2NrcyBzb29uZXIuCisKKyAgICAgICAgKiBoZWFw L0luY3JlbWVudGFsU3dlZXBlci5jcHA6CisgICAgICAgIChKU0M6OkluY3JlbWVudGFsU3dlZXBl cjo6ZG9Td2VlcCk6IENoYW5nZSB0byB1c2Ugc3dlZXBOZXh0QmxvY2suCisgICAgICAgIChKU0Mp OgorICAgICAgICAoSlNDOjpJbmNyZW1lbnRhbFN3ZWVwZXI6OnN3ZWVwTmV4dEJsb2NrKTogCisg ICAgICAgICogaGVhcC9JbmNyZW1lbnRhbFN3ZWVwZXIuaDoKKyAgICAgICAgKEluY3JlbWVudGFs U3dlZXBlcik6CisgICAgICAgICogaGVhcC9NYXJrZWRBbGxvY2F0b3IuY3BwOgorICAgICAgICAo SlNDOjpNYXJrZWRBbGxvY2F0b3I6OnRyeUFsbG9jYXRlSGVscGVyKTogV2hlbiB3ZSBjYW4ndCBz YWZlbHkgc3dlZXAgCisgICAgICAgIG91ciBTdHJ1Y3R1cmUgYmxvY2tzLCBjYWxsIHN3ZWVwTmV4 dEJsb2NrIGluc3RlYWQuCisKIDIwMTItMDctMzEgIFNhbSBXZWluaWcgIDxzYW1Ad2Via2l0Lm9y Zz4KIAogICAgICAgICBGaXggdGhlIFdpbmRvd3MgYnVpbGQuCkluZGV4OiBTb3VyY2UvSmF2YVNj cmlwdENvcmUvaGVhcC9JbmNyZW1lbnRhbFN3ZWVwZXIuY3BwCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJj ZS9KYXZhU2NyaXB0Q29yZS9oZWFwL0luY3JlbWVudGFsU3dlZXBlci5jcHAJKHJldmlzaW9uIDEy NDI2OCkKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9oZWFwL0luY3JlbWVudGFsU3dlZXBlci5j cHAJKHdvcmtpbmcgY29weSkKQEAgLTczLDYgKzczLDIzIEBAIHZvaWQgSW5jcmVtZW50YWxTd2Vl cGVyOjpjYW5jZWxUaW1lcigpCiB2b2lkIEluY3JlbWVudGFsU3dlZXBlcjo6ZG9Td2VlcChkb3Vi bGUgc3dlZXBCZWdpblRpbWUpCiB7CiAgICAgd2hpbGUgKG1fY3VycmVudEJsb2NrVG9Td2VlcElu ZGV4IDwgbV9ibG9ja3NUb1N3ZWVwLnNpemUoKSkgeworICAgICAgICBzd2VlcE5leHRCbG9jaygp OworCisgICAgICAgIENGVGltZUludGVydmFsIGVsYXBzZWRUaW1lID0gV1RGOjptb25vdG9uaWNh bGx5SW5jcmVhc2luZ1RpbWUoKSAtIHN3ZWVwQmVnaW5UaW1lOworICAgICAgICBpZiAoZWxhcHNl ZFRpbWUgPCBzd2VlcFRpbWVTbGljZSkKKyAgICAgICAgICAgIGNvbnRpbnVlOworCisgICAgICAg IHNjaGVkdWxlVGltZXIoKTsKKyAgICAgICAgcmV0dXJuOworICAgIH0KKworICAgIG1fYmxvY2tz VG9Td2VlcC5jbGVhcigpOworICAgIGNhbmNlbFRpbWVyKCk7Cit9CisKK3ZvaWQgSW5jcmVtZW50 YWxTd2VlcGVyOjpzd2VlcE5leHRCbG9jaygpCit7CisgICAgd2hpbGUgKG1fY3VycmVudEJsb2Nr VG9Td2VlcEluZGV4IDwgbV9ibG9ja3NUb1N3ZWVwLnNpemUoKSkgewogICAgICAgICBNYXJrZWRC bG9jayogYmxvY2sgPSBtX2Jsb2Nrc1RvU3dlZXBbbV9jdXJyZW50QmxvY2tUb1N3ZWVwSW5kZXgr K107CiAgICAgICAgIGlmIChibG9jay0+b25seUNvbnRhaW5zU3RydWN0dXJlcygpKQogICAgICAg ICAgICAgbV9zdHJ1Y3R1cmVzQ2FuQmVTd2VwdCA9IHRydWU7CkBAIC04NCwxNyArMTAxLDggQEAg dm9pZCBJbmNyZW1lbnRhbFN3ZWVwZXI6OmRvU3dlZXAoZG91YmxlIAogCiAgICAgICAgIGJsb2Nr LT5zd2VlcCgpOwogICAgICAgICBtX2dsb2JhbERhdGEtPmhlYXAub2JqZWN0U3BhY2UoKS5mcmVl T3JTaHJpbmtCbG9jayhibG9jayk7Ci0KLSAgICAgICAgQ0ZUaW1lSW50ZXJ2YWwgZWxhcHNlZFRp bWUgPSBXVEY6Om1vbm90b25pY2FsbHlJbmNyZWFzaW5nVGltZSgpIC0gc3dlZXBCZWdpblRpbWU7 Ci0gICAgICAgIGlmIChlbGFwc2VkVGltZSA8IHN3ZWVwVGltZVNsaWNlKQotICAgICAgICAgICAg Y29udGludWU7Ci0KLSAgICAgICAgc2NoZWR1bGVUaW1lcigpOwogICAgICAgICByZXR1cm47CiAg ICAgfQotCi0gICAgbV9ibG9ja3NUb1N3ZWVwLmNsZWFyKCk7Ci0gICAgY2FuY2VsVGltZXIoKTsK IH0KIAogdm9pZCBJbmNyZW1lbnRhbFN3ZWVwZXI6OnN0YXJ0U3dlZXBpbmcoY29uc3QgSGFzaFNl dDxNYXJrZWRCbG9jayo+JiBibG9ja1NuYXBzaG90KQpAQCAtMTQzLDYgKzE1MSwxMCBAQCB2b2lk IEluY3JlbWVudGFsU3dlZXBlcjo6d2lsbEZpbmlzaFN3ZWVwCiAgICAgbV9zdHJ1Y3R1cmVzQ2Fu QmVTd2VwdCA9IHRydWU7CiB9CiAKK3ZvaWQgSW5jcmVtZW50YWxTd2VlcGVyOjpzd2VlcE5leHRC bG9jaygpCit7Cit9CisKICNlbmRpZgogCiBib29sIEluY3JlbWVudGFsU3dlZXBlcjo6c3RydWN0 dXJlc0NhbkJlU3dlcHQoKQpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAvSW5jcmVt ZW50YWxTd2VlcGVyLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAv SW5jcmVtZW50YWxTd2VlcGVyLmgJKHJldmlzaW9uIDEyNDI2OCkKKysrIFNvdXJjZS9KYXZhU2Ny aXB0Q29yZS9oZWFwL0luY3JlbWVudGFsU3dlZXBlci5oCSh3b3JraW5nIGNvcHkpCkBAIC01NSw2 ICs1NSw3IEBAIHB1YmxpYzoKICAgICBzdGF0aWMgSW5jcmVtZW50YWxTd2VlcGVyKiBjcmVhdGUo SGVhcCopOwogICAgIHZvaWQgc3RhcnRTd2VlcGluZyhjb25zdCBIYXNoU2V0PE1hcmtlZEJsb2Nr Kj4mIGJsb2NrU25hcHNob3QpOwogICAgIHZpcnR1YWwgdm9pZCBkb1dvcmsoKTsKKyAgICB2b2lk IHN3ZWVwTmV4dEJsb2NrKCk7CiAgICAgYm9vbCBzdHJ1Y3R1cmVzQ2FuQmVTd2VwdCgpOwogICAg IHZvaWQgd2lsbEZpbmlzaFN3ZWVwaW5nKCk7CiAKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29y ZS9oZWFwL01hcmtlZEFsbG9jYXRvci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3Jp cHRDb3JlL2hlYXAvTWFya2VkQWxsb2NhdG9yLmNwcAkocmV2aXNpb24gMTI0MjY4KQorKysgU291 cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAvTWFya2VkQWxsb2NhdG9yLmNwcAkod29ya2luZyBjb3B5 KQpAQCAtMzUsNiArMzUsNyBAQCBpbmxpbmUgdm9pZCogTWFya2VkQWxsb2NhdG9yOjp0cnlBbGxv Y2F0CiAgICAgICAgICAgICAgICAgbV9jdXJyZW50QmxvY2stPmRpZENvbnN1bWVGcmVlTGlzdCgp OwogICAgICAgICAgICAgICAgIG1fY3VycmVudEJsb2NrID0gMDsKICAgICAgICAgICAgIH0KKyAg ICAgICAgICAgIG1faGVhcC0+c3dlZXBlcigpLT5zd2VlcE5leHRCbG9jaygpOwogICAgICAgICAg ICAgcmV0dXJuIDA7CiAgICAgICAgIH0KIAo=