[webkit-reviews] review canceled: [Bug 195000] Add support for incremental bytecode cache updates : [Attachment 362925] Patch

[bugzilla-daemon at webkit.org]

Tadeu Zagallo <tzagallo at apple.com> has canceled Tadeu Zagallo
<tzagallo at apple.com>'s request for review:
Bug 195000: Add support for incremental bytecode cache updates
https://bugs.webkit.org/show_bug.cgi?id=195000

Attachment 362925: Patch

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




--- Comment #5 from Tadeu Zagallo <tzagallo at apple.com> ---
Comment on attachment 362925
  --> https://bugs.webkit.org/attachment.cgi?id=362925
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.