Bug 195000 - Add support for incremental bytecode cache updates
Summary: Add support for incremental bytecode cache updates
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-25 04:17 PST by Tadeu Zagallo
Modified: 2019-04-10 14:19 PDT (History)
9 users (show)

See Also:


Attachments
Patch (46.22 KB, patch)
2019-02-25 07:19 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (46.48 KB, patch)
2019-02-25 12:42 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (47.06 KB, patch)
2019-02-25 13:52 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (50.78 KB, patch)
2019-03-07 05:42 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (52.51 KB, patch)
2019-03-07 09:55 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (52.18 KB, patch)
2019-03-07 11:14 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (53.76 KB, patch)
2019-03-08 00:46 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.60 MB, application/zip)
2019-03-08 02:14 PST, Build Bot
no flags Details
Patch (54.73 KB, patch)
2019-03-15 15:13 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (54.89 KB, patch)
2019-03-18 06:46 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (54.99 KB, patch)
2019-03-19 10:33 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-highsierra (2.53 MB, application/zip)
2019-03-19 15:19 PDT, Build Bot
no flags Details
Patch (64.27 KB, patch)
2019-04-02 02:00 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (66.40 KB, patch)
2019-04-02 13:52 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (66.42 KB, patch)
2019-04-02 15:04 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (69.61 KB, patch)
2019-04-08 02:39 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch for landing (81.89 KB, patch)
2019-04-10 02:16 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (84.89 KB, patch)
2019-04-10 05:30 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (84.33 KB, patch)
2019-04-10 08:26 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (84.00 KB, patch)
2019-04-10 09:31 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2019-02-25 04:17:36 PST
...
Comment 1 Tadeu Zagallo 2019-02-25 07:19:32 PST
Created attachment 362901 [details]
Patch
Comment 2 Tadeu Zagallo 2019-02-25 12:42:55 PST
Created attachment 362918 [details]
Patch

Rebase
Comment 3 Tadeu Zagallo 2019-02-25 13:52:59 PST
Created attachment 362925 [details]
Patch

Fix mac-32bit and windows builds
Comment 4 Saam Barati 2019-02-28 14:28:06 PST
Comment on attachment 362925 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362925&action=review

> Source/JavaScriptCore/ChangeLog:8
> +        Add CacheUpdates to the bytecode to allow to incrementally build the

"CacheUpdates" => "cache updates"

> Source/JavaScriptCore/ChangeLog:11
> +        In order to that, we keep track of the UnlinkedCodeBlocks missing from

"to that" => "to do that"

> Source/JavaScriptCore/jsc.cpp:1000
> +        if (m_cachedBytecode.size())
> +            munmap(const_cast<uint8_t*>(m_cachedBytecode.data()), m_cachedBytecode.size());
> +

How is this correct? This is called from writeCodeBlock, which may be called many times. Maybe I'm missing something? If I'm not, we need tests for this. I suspect this just needs to move back to the destructor.

But this brings up an interesting question, since your mmap is for only the initial size. Maybe you need to mmap to reload at the end of this call? I'm not sure what mmap semantics for size are when you're mmapping a file descriptor.

> Source/JavaScriptCore/jsc.cpp:1005
> +        bool success = m_cachedBytecode.forEachUpdate([&](off_t offset, const void* data, size_t size) {

style nit: space after "]"

> Source/JavaScriptCore/runtime/CachedBytecode.cpp:54
> +    other.m_owned = false;

Should we just null out it's other fields too?

> Source/JavaScriptCore/runtime/CachedBytecode.cpp:157
> +        case CacheUpdate::Function: {

How are we supposed to know all the fields that need to be updated? Is there just some header struct we can just write out all at once in contiguous memory to make this code easier to digest and keep updated?

> Source/JavaScriptCore/runtime/CachedBytecode.cpp:162
> +                // update m_unlinkedCodeBlockFor(Call|Construct).m_ptr.m_offset

worth a static assert that this field is ptrdiff_t

> Source/JavaScriptCore/runtime/CachedBytecode.cpp:190
> +        } }

style nit: should be on different lines

> Source/JavaScriptCore/runtime/CachedBytecode.cpp:200
> +    ASSERT(static_cast<size_t>(offset) == m_size);
> +

Do we not need to update m_payload here?

> Source/JavaScriptCore/runtime/CachedBytecode.h:132
> +    Type m_type;
> +    union {
> +        GlobalUpdate m_globalUpdate;
> +        FunctionUpdate m_functionUpdate;
> +    };

WTF::Variant?

> Source/JavaScriptCore/runtime/CachedTypes.cpp:141
> +    void addLeafCodeBlock(const UnlinkedFunctionExecutable* executable, ptrdiff_t offset)
> +    {
> +        m_leafCodeBlocks.add(executable, offset);
> +    }

This naming is weird given that it's an executable.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:350
> +

style nit: we don't really need a newline here

> Source/JavaScriptCore/runtime/CachedTypes.cpp:1860
> +        ASSERT(codeBlock->isStrictMode());

Why?

> Source/JavaScriptCore/runtime/CachedTypes.cpp:2010
> +

remove

> Source/JavaScriptCore/runtime/CachedTypes.cpp:2015
> +        encoder.addLeafCodeBlock(&executable, encoder.offsetOf(this));

nit: Not this patch, but we tend to use pointers instead of references for GC pointers just as a style policy in JSC.
Comment 5 Tadeu Zagallo 2019-02-28 23:22:35 PST
Comment on attachment 362925 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362925&action=review

>> Source/JavaScriptCore/jsc.cpp:1000
>> +
> 
> How is this correct? This is called from writeCodeBlock, which may be called many times. Maybe I'm missing something? If I'm not, we need tests for this. I suspect this just needs to move back to the destructor.
> 
> But this brings up an interesting question, since your mmap is for only the initial size. Maybe you need to mmap to reload at the end of this call? I'm not sure what mmap semantics for size are when you're mmapping a file descriptor.

I think I need to clear the CachedBytecode here. I also realized that we probably won't update the cache, since we are checking for SourceCodeValue::written, and we never clear it.

I'll move the mmap to cachedBytecode(), that way it will be reloaded if we try to read it again, does that make sense?

>> Source/JavaScriptCore/runtime/CachedBytecode.cpp:54
>> +    other.m_owned = false;
> 
> Should we just null out it's other fields too?

Sure, I can do that.

>> Source/JavaScriptCore/runtime/CachedBytecode.cpp:157
>> +        case CacheUpdate::Function: {
> 
> How are we supposed to know all the fields that need to be updated? Is there just some header struct we can just write out all at once in contiguous memory to make this code easier to digest and keep updated?

Yeah, I'm not happy about this either. I'll try to move all of this into a struct as you suggested.

>> Source/JavaScriptCore/runtime/CachedBytecode.cpp:162
>> +                // update m_unlinkedCodeBlockFor(Call|Construct).m_ptr.m_offset
> 
> worth a static assert that this field is ptrdiff_t

that's a good idea.

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:141
>> +    }
> 
> This naming is weird given that it's an executable.

Yeah, I was recording the code blocks at first. I'll update it.

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:1860
>> +        ASSERT(codeBlock->isStrictMode());
> 
> Why?

Oops, this is leftover from testing.

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:2015
>> +        encoder.addLeafCodeBlock(&executable, encoder.offsetOf(this));
> 
> nit: Not this patch, but we tend to use pointers instead of references for GC pointers just as a style policy in JSC.

This is how the cache hierarchy was designed: A UnlinkedFunctionExecutable* is cached as CachedPtr<CachedFunctionExecutable>. The way CachedPtr works is that it takes a T*, and encodes it with `encode(*T)`, so all the encode functions take a reference.
Comment 6 Tadeu Zagallo 2019-03-07 05:42:23 PST
Created attachment 363869 [details]
Patch
Comment 7 Tadeu Zagallo 2019-03-07 09:55:33 PST
Created attachment 363887 [details]
Patch
Comment 8 Tadeu Zagallo 2019-03-07 11:14:06 PST
Created attachment 363903 [details]
Patch
Comment 9 Tadeu Zagallo 2019-03-08 00:46:37 PST
Created attachment 363995 [details]
Patch
Comment 10 Build Bot 2019-03-08 02:14:04 PST
Comment on attachment 363995 [details]
Patch

Attachment 363995 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11424742

New failing tests:
accessibility/mac/selection-notification-focus-change.html
Comment 11 Build Bot 2019-03-08 02:14:05 PST
Created attachment 363998 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 12 Saam Barati 2019-03-14 20:14:44 PDT
Comment on attachment 363995 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363995&action=review

> Source/JavaScriptCore/ChangeLog:14
> +        bytecode. The CodeBlock to be cached is then encoded on its own buffer

nit:
CodeBlock => UnlinkedCodeBlock
on => in

> Source/JavaScriptCore/ChangeLog:15
> +        and later appended to the existing cache.

I think it's worth hammering this home in another sentence: incremental updates work because we basically purely append to the existing cache + update a few fixed fields in the existing cache

> Source/JavaScriptCore/jsc.cpp:1003
> +            m_cachedBytecode.clear();

do we ever set up a new m_cachedBytecode?

> Source/JavaScriptCore/runtime/CachedBytecode.cpp:121
> +    // global code block

can we just each of these functions better names?
like addGlobalUpdate and addFunctionUpdate?

> Source/JavaScriptCore/runtime/CachedBytecode.cpp:145
> +bool CachedBytecode::forEachUpdate(const ForEachUpdateCallback& callback) const

This is a weird name for this function since it's about success. Maybe "bool commitUpdates(...)"?

> Source/JavaScriptCore/runtime/CachedBytecode.cpp:147
> +    off_t offset = m_payload.size();

Do we ever update m_payload?

> Source/JavaScriptCore/runtime/CachedBytecode.cpp:155
> +            ptrdiff_t kindOffset = functionUpdate.m_kind == CodeForCall ? CachedFunctionExecutableOffsets::codeBlockForCallOffset() : CachedFunctionExecutableOffsets::codeBlockForConstructOffset();

style nit: this should go in the below block.

> Source/JavaScriptCore/runtime/CachedBytecode.cpp:157
> +                // update m_unlinkedCodeBlockFor(Call|Construct).m_ptr.m_offset

nit: I don't think these comments are adding anything, since the code makes it obvious what's going on.

> Source/JavaScriptCore/runtime/CachedBytecode.cpp:160
> +                if (!callback(codeBlockOffset, &offsetPayload, sizeof(ptrdiff_t)))

you should static assert whatever field this is is sizeof(ptrdiff_t)
Comment 13 Tadeu Zagallo 2019-03-15 12:07:20 PDT
Comment on attachment 363995 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363995&action=review

>> Source/JavaScriptCore/jsc.cpp:1003
>> +            m_cachedBytecode.clear();
> 
> do we ever set up a new m_cachedBytecode?

Yes, in `loadBytecode`.

>> Source/JavaScriptCore/runtime/CachedBytecode.cpp:121
>> +    // global code block
> 
> can we just each of these functions better names?
> like addGlobalUpdate and addFunctionUpdate?

Sure, I will do that.

>> Source/JavaScriptCore/runtime/CachedBytecode.cpp:147
>> +    off_t offset = m_payload.size();
> 
> Do we ever update m_payload?

No, `m_payload` is only set in the constructor.
Comment 14 Tadeu Zagallo 2019-03-15 15:13:47 PDT
Created attachment 364854 [details]
Patch
Comment 15 Tadeu Zagallo 2019-03-18 06:46:58 PDT
Created attachment 365013 [details]
Patch

Rebase and update ChangeLog
Comment 16 Saam Barati 2019-03-18 14:29:02 PDT
Comment on attachment 365013 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365013&action=review

> Source/JavaScriptCore/runtime/CachedBytecode.cpp:139
> +        auto addResult = m_leafExecutables.add(it.key, it.value + m_size);

I'm a little bit lost on seeing why this is the correct offset math. Can you explain why this works?

> Source/JavaScriptCore/runtime/CachedBytecode.cpp:145
> +bool CachedBytecode::commitUpdates(const ForEachUpdateCallback& callback) const

What happens when one of the later updates fails but an earlier one doesn't? Don't we end up in some weird corrupted state? E.g, what if we successfully update the offset but fail to write the payload the offset points to?

Maybe the right API to use is to reserve the final file size. If that fails, we bail. If that succeeds, we can just write everything out to disk assuming success?

I think we should find a way to make this update atomic.

> Source/JavaScriptCore/runtime/CachedBytecode.cpp:171
> +        // write the payload

nit: remove comment
Comment 17 Tadeu Zagallo 2019-03-18 14:34:17 PDT
Comment on attachment 365013 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365013&action=review

>> Source/JavaScriptCore/runtime/CachedBytecode.cpp:139
>> +        auto addResult = m_leafExecutables.add(it.key, it.value + m_size);
> 
> I'm a little bit lost on seeing why this is the correct offset math. Can you explain why this works?

The leaf executable was recorded while encoding the function in its own buffer, so when we append to the cache we need to offset it by the current size of the cache so that it's relative to the start of the file.

>> Source/JavaScriptCore/runtime/CachedBytecode.cpp:145
>> +bool CachedBytecode::commitUpdates(const ForEachUpdateCallback& callback) const
> 
> What happens when one of the later updates fails but an earlier one doesn't? Don't we end up in some weird corrupted state? E.g, what if we successfully update the offset but fail to write the payload the offset points to?
> 
> Maybe the right API to use is to reserve the final file size. If that fails, we bail. If that succeeds, we can just write everything out to disk assuming success?
> 
> I think we should find a way to make this update atomic.

Right now we just truncate the file if any of the updates fail, but I agree your solution is better. I'll update it.
Comment 18 Tadeu Zagallo 2019-03-19 10:33:41 PDT
Created attachment 365181 [details]
Patch
Comment 19 Saam Barati 2019-03-19 11:54:29 PDT
Comment on attachment 365013 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365013&action=review

>>> Source/JavaScriptCore/runtime/CachedBytecode.cpp:139
>>> +        auto addResult = m_leafExecutables.add(it.key, it.value + m_size);
>> 
>> I'm a little bit lost on seeing why this is the correct offset math. Can you explain why this works?
> 
> The leaf executable was recorded while encoding the function in its own buffer, so when we append to the cache we need to offset it by the current size of the cache so that it's relative to the start of the file.

Just to verify I'm understanding this correctly: this is dependent on the order of m_updates too, right?
Comment 20 Tadeu Zagallo 2019-03-19 13:41:07 PDT
Comment on attachment 365013 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365013&action=review

>>>> Source/JavaScriptCore/runtime/CachedBytecode.cpp:139
>>>> +        auto addResult = m_leafExecutables.add(it.key, it.value + m_size);
>>> 
>>> I'm a little bit lost on seeing why this is the correct offset math. Can you explain why this works?
>> 
>> The leaf executable was recorded while encoding the function in its own buffer, so when we append to the cache we need to offset it by the current size of the cache so that it's relative to the start of the file.
> 
> Just to verify I'm understanding this correctly: this is dependent on the order of m_updates too, right?

Yes, that's correct.
Comment 21 Build Bot 2019-03-19 15:19:53 PDT
Comment on attachment 365181 [details]
Patch

Attachment 365181 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11570460

New failing tests:
http/tests/inspector/dom/didFireEvent.html
Comment 22 Build Bot 2019-03-19 15:19:55 PDT
Created attachment 365239 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 23 Saam Barati 2019-03-27 18:26:13 PDT
Comment on attachment 365181 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365181&action=review

Can you file a follow-up patch to hook the ObjC API into incremental caching?

Mostly LGTM, one final question/comment.

> Source/JavaScriptCore/runtime/CodeCache.cpp:205
>      if (!codeBlock)
>          return;

When is this false? Why not just make this only take key?

This function now feels slightly weird since we may call commitCachedBytecode repeatedly on the same provider. Is that expected? It seems inefficient.
Comment 24 Tadeu Zagallo 2019-03-28 10:22:46 PDT
Comment on attachment 365181 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365181&action=review

>> Source/JavaScriptCore/runtime/CodeCache.cpp:205
>>          return;
> 
> When is this false? Why not just make this only take key?
> 
> This function now feels slightly weird since we may call commitCachedBytecode repeatedly on the same provider. Is that expected? It seems inefficient.

This is false for the UnlinkedFunctionExecutable entries in the cache. We can't take just the key because the CodeBlock is the value and we need the VM for the jsDynamicCast. (We could get the VM for the cell, but it seemed inefficient in the places that we already have it.)

I don't expect us to call it multiple times on the same provider. We only cache UnlinkedGlobalCodeBlocks, is it possible/common to have the same provider for multiple global code blocks? Unless it's very common, it should be ok since the provider will clear the updates after successfully committing them, so it should be a no-op.
Comment 25 Saam Barati 2019-03-28 11:49:20 PDT
Comment on attachment 365181 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365181&action=review

> Source/JavaScriptCore/jsc.cpp:1000
> +            munmap(const_cast<uint8_t*>(m_cachedBytecode.data()), m_cachedBytecode.size());

As mentioned on IRC, this may be problematic.

>>> Source/JavaScriptCore/runtime/CodeCache.cpp:205
>>>          return;
>> 
>> When is this false? Why not just make this only take key?
>> 
>> This function now feels slightly weird since we may call commitCachedBytecode repeatedly on the same provider. Is that expected? It seems inefficient.
> 
> This is false for the UnlinkedFunctionExecutable entries in the cache. We can't take just the key because the CodeBlock is the value and we need the VM for the jsDynamicCast. (We could get the VM for the cell, but it seemed inefficient in the places that we already have it.)
> 
> I don't expect us to call it multiple times on the same provider. We only cache UnlinkedGlobalCodeBlocks, is it possible/common to have the same provider for multiple global code blocks? Unless it's very common, it should be ok since the provider will clear the updates after successfully committing them, so it should be a no-op.

Got it. Makes sense. I agree we shouldn't have the same provider for different global code blocks.

However, your commit doesn't seem like it'll be a nop  if called multiple times in a row.
Comment 26 Tadeu Zagallo 2019-04-02 02:00:59 PDT
Created attachment 366478 [details]
Patch
Comment 27 Tadeu Zagallo 2019-04-02 13:52:38 PDT
Created attachment 366533 [details]
Patch

Fix build
Comment 28 Tadeu Zagallo 2019-04-02 15:04:59 PDT
Created attachment 366542 [details]
Patch

Fix windows build
Comment 29 Saam Barati 2019-04-03 17:42:41 PDT
Comment on attachment 366542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366542&action=review

> Source/JavaScriptCore/API/JSScript.mm:53
> +    RefPtr<JSC::CachedBytecode> m_cachedBytecode;

Why not Ref? When is this ever null?

Or why not make it null to represent not having a cache? Why create the empty BytecodeCache instead of just keeping null to signify the same thing?

> Source/JavaScriptCore/jsc.cpp:995
> +    void commitCachedBytecode() const override

feels like a weird function to be marked as const given the name and the desired semantics.

Also, I'm still confused on what happens if this gets called twice? If we end up clearing this in the makeScopeExit below, how will the second call succeed? Maybe the way other things are structured says this won't happen, but I feel like we should be able to handle it. Logically, I feel like I should be able to repeatedly call commitCachedBytecode on the same source provider.

> Source/JavaScriptCore/jsc.cpp:1002
> +            m_cachedBytecode = CachedBytecode::create();

same comment as earlier. Why not just use nullptr to represent this?

> Source/JavaScriptCore/jsc.cpp:1021
> +            size_t bytesWritten = static_cast<size_t>(write(fd, data, size));
> +            ASSERT_UNUSED(bytesWritten, bytesWritten == size);

this feels like a dangerous thing to assert. Does POSIX guarantee this succeeds if the file is of sufficient size?

> Source/JavaScriptCore/jsc.cpp:1081
> +    mutable Ref<CachedBytecode> m_cachedBytecode;

if you unmark the commit function as const you can also remove this mutable.

> Source/JavaScriptCore/runtime/CachedBytecode.cpp:63
> +#if OS(DARWIN)
> +        munmap(m_data, m_size);
> +#endif

Should this be:
#else
ASSERT_NOT_REACHED() or similar?

That said, Darwin feels weird here. Why not just do this for posix?

I also think that means the constructor that says mapped should be compiled out for non posix? Or crash for non posix?

> Source/JavaScriptCore/runtime/CachedBytecode.cpp:93
> +    this->~CacheUpdate();

you should add:
ASSERT(&other != this)

> Source/JavaScriptCore/runtime/CachedBytecode.h:61
> +    CachePayload(void* data, size_t size)
> +        : m_mapped(true)
> +        , m_size(size)
> +        , m_data(static_cast<uint8_t*>(data))
> +    {
> +    }
> +
> +    CachePayload(MallocPtr<uint8_t>&& data, size_t size)
> +        : m_mapped(false)
> +        , m_size(size)
> +        , m_data(data.leakPtr())
> +    {
> +    }

nit: I think having static functions with names is a bit better to read at the callsite. e.g,

static CachePayload makeMappedPayload(void*, size_t)

> Source/JavaScriptCore/runtime/CachedBytecode.h:156
> +    size_t sizeForUpdate() { return m_size; }

style nit: can be marked as const

> Source/JavaScriptCore/runtime/CodeCache.h:170
> +        VERBOSE_LOG("Found cached CodeBlock in the SourceProvider");

I know this was here before, but do we really want to keep this?
Comment 30 Tadeu Zagallo 2019-04-08 01:01:57 PDT
Comment on attachment 366542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366542&action=review

>> Source/JavaScriptCore/API/JSScript.mm:53
>> +    RefPtr<JSC::CachedBytecode> m_cachedBytecode;
> 
> Why not Ref? When is this ever null?
> 
> Or why not make it null to represent not having a cache? Why create the empty BytecodeCache instead of just keeping null to signify the same thing?

The only reason this is `RefPtr` instead of `Ref` is because there's no default initializer for `Ref` and I believe there's no way to initialize it from an Obj-C class.

I believe we could use `null` to represent not having the cache for now, but it doesn't work as a general solution because we need to append the updates to the empty cache.

>> Source/JavaScriptCore/jsc.cpp:995
>> +    void commitCachedBytecode() const override
> 
> feels like a weird function to be marked as const given the name and the desired semantics.
> 
> Also, I'm still confused on what happens if this gets called twice? If we end up clearing this in the makeScopeExit below, how will the second call succeed? Maybe the way other things are structured says this won't happen, but I feel like we should be able to handle it. Logically, I feel like I should be able to repeatedly call commitCachedBytecode on the same source provider.

All the cache functions had to be marked `const` and `m_cachedBytecode` because we can only have a const reference to the source provider. I'm not happy about it, but couldn't find a better solution.

The second call will bail because after clearing `m_cachedBytecode`, hasUpdates will return false in the first check. I added this after your comment in the last patch, and I think it should handle multiple calls.

>> Source/JavaScriptCore/jsc.cpp:1002
>> +            m_cachedBytecode = CachedBytecode::create();
> 
> same comment as earlier. Why not just use nullptr to represent this?

Here is the case where we can't use null, because here we actually support updates. We could have an `ensureCachedBytecode()` helper, but it felt like overkill.

>> Source/JavaScriptCore/jsc.cpp:1021
>> +            ASSERT_UNUSED(bytesWritten, bytesWritten == size);
> 
> this feels like a dangerous thing to assert. Does POSIX guarantee this succeeds if the file is of sufficient size?

I couldn't find any reason in the man pages why it would fail for a file if there's sufficient space, but I'm honestly not sure. Either way, if this is possible, we're in a bad place, because we're no longer checking for successful writes of every chunk and we don't do any integrity check in the bytecode cache. If you think this is dangerous, I believe we'll have to rollback to my previous version where every write returns where it was successful.

>> Source/JavaScriptCore/jsc.cpp:1081
>> +    mutable Ref<CachedBytecode> m_cachedBytecode;
> 
> if you unmark the commit function as const you can also remove this mutable.

see my comment above that we can only find the SourceProvider through a const reference.

>> Source/JavaScriptCore/runtime/CachedBytecode.cpp:63
>> +#endif
> 
> Should this be:
> #else
> ASSERT_NOT_REACHED() or similar?
> 
> That said, Darwin feels weird here. Why not just do this for posix?
> 
> I also think that means the constructor that says mapped should be compiled out for non posix? Or crash for non posix?

Technically you could still use it, but it would be up to the caller to free the data, which as I write I realize wouldn't be very useful. I'll remove the constructor and change it to posix.
Comment 31 Tadeu Zagallo 2019-04-08 02:39:59 PDT
Created attachment 366923 [details]
Patch
Comment 32 Filip Pizlo 2019-04-09 14:24:50 PDT
Comment on attachment 366923 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366923&action=review

r=me but it seems like this needs to be rebased and please try to put classes in separate files.

> Source/JavaScriptCore/runtime/CachedBytecode.h:42
> +class CachePayload {

Ideally this would be a separate header.  And the implementation in a separate .cpp file.

> Source/JavaScriptCore/runtime/CachedBytecode.h:72
> +class LeafExecutable {

Ditto.
Comment 33 Tadeu Zagallo 2019-04-10 02:16:42 PDT
Created attachment 367111 [details]
Patch for landing

I'll just wait for EWS to confirm that all platforms are green
Comment 34 Tadeu Zagallo 2019-04-10 05:30:54 PDT
Created attachment 367114 [details]
Patch

Fix build
Comment 35 Tadeu Zagallo 2019-04-10 08:26:01 PDT
Created attachment 367124 [details]
Patch

Try to fix the build again
Comment 36 Tadeu Zagallo 2019-04-10 09:31:45 PDT
Created attachment 367128 [details]
Patch

Try to fix the build yet another time
Comment 37 WebKit Commit Bot 2019-04-10 12:18:27 PDT
Comment on attachment 367128 [details]
Patch

Clearing flags on attachment: 367128

Committed r244143: <https://trac.webkit.org/changeset/244143>
Comment 38 WebKit Commit Bot 2019-04-10 12:18:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 39 Radar WebKit Bug Importer 2019-04-10 12:19:23 PDT
<rdar://problem/49785261>
Comment 40 Tadeu Zagallo 2019-04-10 14:15:29 PDT
Committed r244149: <https://trac.webkit.org/changeset/244149>
Comment 41 Tadeu Zagallo 2019-04-10 14:19:22 PDT
Ugh, webkit-patch automatically commented... r244149 is a fix for the watch builds.