Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed #14053 #3

Merged
merged 2 commits into from
May 5, 2018
Merged

Fixed #14053 #3

merged 2 commits into from
May 5, 2018

Conversation

vchrombie
Copy link

Fixed #14053
also updated the README with a Note regarding the Ruby missing error.

@vchrombie
Corrected the base color of the Scrollbar
8b4766d 
Corrected the base color of the Scrollbar according to the Appearance Settings.
@vchrombie
Update README.markdown
455d94c 
Updated README.markdown with a suggestion for Ruby missing error.
pulkomandy pushed a commit that referenced this pull request Apr 7, 2018
REGRESSION(r212513): LastResort is platform-dependent, so its semanti…
0113371 
…cs should not be required to perform font loading correctly.

https://bugs.webkit.org/show_bug.cgi?id=168487

Reviewed by Antti Koivisto.

Source/WebCore:

There are three ways a Web author can chain multiple font files together:
1. Multiple entries in the "src" descriptor in an @font-face rule
2. Multiple @font-face rules with the same "font-family" descriptor
3. Multiple entries in the "font-family" property on an element

Before r212513, the code which iterated across #2 and #3 above could have
triggered each item in the chain to download. r212513 tried to solve this
by using LastResort as the interstitial font used during downloads, because
LastResort supports every character and therefore solves #3 above. However,
this change had a few problems:

1. Previously, our code would try to avoid using the interstitial font for
layout or rendering whenever possible (because one of the chains above may
have named a local font which would be better to use). In order to use the
benefits of LastResort, I had to remove this avoidance logic and make
WebKit try to use the interstitial font as often as possible. However, due
to the large metrics of LastResort, this means that offsetWidth queries
during font loading would be wildly inaccurate, causing Google Docs to break.
2. It also means that canvas drawing during font loading would actually draw
LastResort, causing Bing maps to break.
3. LastResort is platform-specific, so only platforms which have it would
actually be able to load fonts correctly.

Instead, we should keep the older logic about avoiding using the
interstitial font so that loading has a better experience for the user.
We solve the unnecessary download problem by giving our loading code a
downloading policy enum, which has two values: allow downloads or forbid
downloads. Whenever our loading code returns the interstitial font, we
continue our search, but we change the policy to forbid downloads.

There is one piece of subtlety, though: It is more common for web authors
to put good fallbacks in the "font-family" property than in the "src"
descriptor inside @font-face. This means that we shouldn't exhaustively
search through the @font-face src list first. Instead, we should look
through the src list until we hit a non-local font, and then immediately
start looking through the other other chains.

Tests: fast/text/font-download-font-face-src-list.html
       fast/text/font-download-font-family-property.html
       fast/text/font-download-remote-fallback-all.html
       fast/text/font-interstitial-invisible-width-while-loading.html
       fast/text/font-weight-download-3.html
       fast/text/web-font-load-fallback-during-loading-2.html
       fast/text/web-font-load-invisible-during-loading.html

* css/CSSFontFace.cpp:
(WebCore::CSSFontFace::fontLoadEventOccurred): Implement support for
the font download policy.
(WebCore::CSSFontFace::setStatus): After 3 seconds of loading, we
will start drawing the fallback font. However, for testing, we have an
internal setting to make this switch happen immediately. This patch now
requires that this internal switch happen synchronously.
(WebCore::CSSFontFace::pump): Implement support for the font download
policy.
(WebCore::CSSFontFace::load): Ditto.
(WebCore::CSSFontFace::font): Ditto.
* css/CSSFontFace.h: Ditto.
* css/CSSFontSelector.cpp:
(WebCore::CSSFontSelector::beginLoadingFontSoon): Implement support for
synchronous font download timeouts.
* css/CSSSegmentedFontFace.cpp:
(WebCore::CSSSegmentedFontFace::fontRanges): Implement support for the
font download policy.
* platform/graphics/Font.cpp: Add new flag which represents if the
interstitial font was created after the 3 second timeout or before.
Previously, we would distinguish between these two cases by knowing
that one font was LastResort and the other font was a fallback. Now that
we're using fallback fonts on both sides of the 3 second timeout, we
now no longer know which one should be invisible. This new enum solves
this problem.
(WebCore::Font::Font):
(WebCore::Font::verticalRightOrientationFont):
(WebCore::Font::uprightOrientationFont):
* platform/graphics/Font.h: Ditto.
(WebCore::Font::create):
(WebCore::Font::origin):
(WebCore::Font::visibility):
* platform/graphics/FontCache.h:
* platform/graphics/FontCascade.cpp: We try to fall back to a local() font
during downloads, but there might not be one that we can use. Therefore, we
can't use the presence of the interstitial font to detect if we should paint
invisibly. Instead, we can move this logic into the font-specific part of
painting, and consult with the specific font to know if it was created from
a timed-out @font-face rule or not.
(WebCore::FontCascade::drawText):
(WebCore::shouldDrawIfLoading):
(WebCore::FontCascade::drawGlyphBuffer):
(WebCore::FontCascade::drawEmphasisMarks):
* platform/graphics/FontCascade.h:
* platform/graphics/FontCascadeFonts.cpp:
(WebCore::FontCascadeFonts::glyphDataForVariant): Implement the logic
described above where we switch the policy if we encounter the intestitial
font.
(WebCore::FontCascadeFonts::glyphDataForNormalVariant): Ditto.
(WebCore::glyphPageFromFontRanges): Ditto.
* platform/graphics/FontRanges.cpp: Implement support for the font download
policy.
(WebCore::FontRanges::Range::font):
(WebCore::FontRanges::glyphDataForCharacter):
(WebCore::FontRanges::fontForCharacter):
(WebCore::FontRanges::fontForFirstRange):
* platform/graphics/FontRanges.h:
* platform/graphics/FontSelector.h:
* platform/graphics/freetype/FontCacheFreeType.cpp:
(WebCore::FontCache::lastResortFallbackFontForEveryCharacter): Deleted.
* platform/graphics/mac/FontCacheMac.mm:
(WebCore::FontCache::lastResortFallbackFontForEveryCharacter): Deleted.
* platform/graphics/win/FontCacheWin.cpp:
(WebCore::FontCache::lastResortFallbackFontForEveryCharacter): Deleted.

LayoutTests:

* fast/text/font-download-font-face-src-list-expected.txt: Added.
* fast/text/font-download-font-face-src-list.html: Copied from LayoutTests/fast/text/font-weight-download-2.html.
* fast/text/font-download-font-family-property-expected.txt: Added.
* fast/text/font-download-font-family-property.html: Copied from LayoutTests/fast/text/font-weight-download-2.html.
* fast/text/font-download-remote-fallback-all-expected.txt: Added.
* fast/text/font-download-remote-fallback-all.html: Copied from LayoutTests/fast/text/font-weight-download-2.html.
* fast/text/font-interstitial-invisible-width-while-loading-expected.txt: Added.
* fast/text/font-interstitial-invisible-width-while-loading.html: Added.
* fast/text/font-weight-download-2.html:
* fast/text/font-weight-download-3-expected.txt: Added.
* fast/text/font-weight-download-3.html: Copied from LayoutTests/fast/text/font-weight-download-2.html.
* fast/text/web-font-load-fallback-during-loading-2-expected.html: Added.
* fast/text/web-font-load-fallback-during-loading-2.html: Added.
* fast/text/web-font-load-fallback-during-loading-expected.html:
* fast/text/web-font-load-fallback-during-loading.html:
* fast/text/web-font-load-invisible-during-loading-expected.txt: Added.
* fast/text/web-font-load-invisible-during-loading.html: Added.
* http/tests/webfont/fallback-font-while-loading-expected.txt:
* http/tests/webfont/fallback-font-while-loading.html:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@216944 268f45cc-cd09-0410-ab3c-d52691b4dbfc
pulkomandy pushed a commit that referenced this pull request Apr 8, 2018
We should not claim that SpecEmpty is filtered out of cell checks on …
5280265 
…64 bit platforms

https://bugs.webkit.org/show_bug.cgi?id=172957
<rdar://problem/32602704>

Reviewed by Filip Pizlo.

JSTests:

* stress/spec-empty-flows-through-cell-checks.js: Added.
(A):
(B):
(i.catch):

Source/JavaScriptCore:

Consider this program:
```
block#1:
n: GetClosureVar(..., |this|) // this will load empty JSValue()
SetLocal(Cell:@n, locFoo) // Cell check succeeds because JSValue() looks like a cell
Branch(#2, #3)

Block#3:
x: GetLocal(locFoo)
y: CheckNotEmpty(@x)
```

If we claim that a cell check filters out the empty value, we will
incorrectly eliminate the CheckNotEmpty node @y. This patch fixes AI,
FTLLowerDFGToB3, and DFGSpeculativeJIT to no longer make this claim.

On 64 bit platforms:
- Cell use kind *now allows* the empty value to pass through.
- CellOrOther use kind *now allows* for the empty value to pass through
- NotCell use kind *no longer allows* the empty value to pass through.

* assembler/CPU.h:
(JSC::isARMv7IDIVSupported):
(JSC::isARM64):
(JSC::isX86):
(JSC::isX86_64):
(JSC::is64Bit):
(JSC::is32Bit):
(JSC::isMIPS):
Make these functions constexpr so we can use them in static variable assignment.

* bytecode/SpeculatedType.h:
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileValueToInt32):
(JSC::DFG::SpeculativeJIT::compileDoubleRep):
(JSC::DFG::SpeculativeJIT::compileLogicalNotStringOrOther):
(JSC::DFG::SpeculativeJIT::emitStringOrOtherBranch):
(JSC::DFG::SpeculativeJIT::speculateCell):
(JSC::DFG::SpeculativeJIT::speculateCellOrOther):
(JSC::DFG::SpeculativeJIT::speculateObjectOrOther):
(JSC::DFG::SpeculativeJIT::speculateString):
(JSC::DFG::SpeculativeJIT::speculateStringOrOther):
(JSC::DFG::SpeculativeJIT::speculateSymbol):
(JSC::DFG::SpeculativeJIT::speculateNotCell):
* dfg/DFGSpeculativeJIT32_64.cpp:
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::fillSpeculateCell):
(JSC::DFG::SpeculativeJIT::compileObjectToObjectOrOtherEquality):
(JSC::DFG::SpeculativeJIT::compilePeepHoleObjectToObjectOrOtherEquality):
(JSC::DFG::SpeculativeJIT::compileObjectOrOtherLogicalNot):
(JSC::DFG::SpeculativeJIT::emitObjectOrOtherBranch):
* dfg/DFGUseKind.h:
(JSC::DFG::typeFilterFor):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileDoubleRep):
(JSC::FTL::DFG::LowerDFGToB3::numberOrNotCellToInt32):
(JSC::FTL::DFG::LowerDFGToB3::compareEqObjectOrOtherToObject):
(JSC::FTL::DFG::LowerDFGToB3::boolify):
(JSC::FTL::DFG::LowerDFGToB3::equalNullOrUndefined):
(JSC::FTL::DFG::LowerDFGToB3::lowCell):
(JSC::FTL::DFG::LowerDFGToB3::lowNotCell):
(JSC::FTL::DFG::LowerDFGToB3::isCellOrMisc):
(JSC::FTL::DFG::LowerDFGToB3::isNotCellOrMisc):
(JSC::FTL::DFG::LowerDFGToB3::isNotCell):
(JSC::FTL::DFG::LowerDFGToB3::isCell):
(JSC::FTL::DFG::LowerDFGToB3::speculateCellOrOther):
(JSC::FTL::DFG::LowerDFGToB3::speculateObjectOrOther):
(JSC::FTL::DFG::LowerDFGToB3::speculateString):
(JSC::FTL::DFG::LowerDFGToB3::speculateStringOrOther):
(JSC::FTL::DFG::LowerDFGToB3::speculateSymbol):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@218137 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@pulkomandy pulkomandy merged commit e068a0b into haiku:rebased May 5, 2018
pulkomandy pushed a commit that referenced this pull request Jun 18, 2018
Octane/richards regressed by a whopping 20% because eliminateCommonSu…
52728ed 
…bexpressions has a weird fixpoint requirement

https://bugs.webkit.org/show_bug.cgi?id=180783

Reviewed by Saam Barati.
        
This fixes the regression by fixpointing CSE. We need to fixpoint CSE because of this case:
        
    BB#1:
        a: Load(@x)
        b: Load(@x)
        c: Load(@b)
    BB#2:
        d: Load(@b)
    BB#3:
        e: Load(@b)
        
Lets assume that #3 loops around to #2, so to eliminate @d, we need to prove that it's redundant
with both @c and @e. The problem is that by the time we get to @d, the CSE state will look like
this:

    BB#1:
        a: Load(@x)
        b: Load(@x)
        c: Load(@A)
        memoryAtTail: {@x=>@A, @A=>@c}
    BB#2:
        d: Load(@A) [sic]
        memoryAtTail: {@b=>@d}
    BB#3:
        e: Load(@b)
        memoryAtTail: {@b=>@e} [sic]
        
Note that #3's atTail map is keyed on @b, which was the old (no longer canonical) version of @A.
But @d's children were already substituted, so it refers to @A. Since @A is not in #3's atTail
map, we don't find it and leave the redundancy.
        
I think that the cleanest solution is to fixpoint. CSE is pretty cheap, so hopefully we can afford
this. It fixes the richards regression, since richards is super dependent on B3 CSE.

* b3/B3EliminateCommonSubexpressions.cpp: Logging.
* b3/B3Generate.cpp:
(JSC::B3::generateToAir): Fix the bug.
* b3/air/AirReportUsedRegisters.cpp:
(JSC::B3::Air::reportUsedRegisters): Logging.
* dfg/DFGByteCodeParser.cpp:
* dfg/DFGSSAConversionPhase.cpp:
(JSC::DFG::SSAConversionPhase::run): Don't generate EntrySwitch if we don't need it (makes IR easier to read).
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::lower): Don't generate EntrySwitch if we don't need it (makes IR easier to read).



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@225893 268f45cc-cd09-0410-ab3c-d52691b4dbfc
pulkomandy pushed a commit that referenced this pull request Oct 23, 2018
[WPE][GTK] Unreviewed, try #3 to fix a typo
030d3f1 
I feel like Charlie Brown, how hard this has been for me....

* Scripts/run-minibrowser:
* Scripts/webkitdirs.pm:
(runInFlatpakIfAvailable):
(runInFlatpakIfAvailible): Deleted.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@236206 268f45cc-cd09-0410-ab3c-d52691b4dbfc
pulkomandy pushed a commit that referenced this pull request Nov 2, 2018
lowXYZ in FTLLower should always filter the type of the incoming edge
8946412 
https://bugs.webkit.org/show_bug.cgi?id=189939
<rdar://problem/44407030>

Reviewed by Michael Saboff.

JSTests:

* stress/ftl-should-always-filter-for-low-type-check-functions.js: Added.
(foo):
(test):

Source/JavaScriptCore:

For example, the FTL may know more about data flow than AI in certain programs,
and it needs to inform AI of these data flow properties to appease the assertion
we have in AI that a node must perform type checks on its child nodes.

For example, consider this program:

```
bb#1
a: Phi // Let's say it has an Int32 result, so it goes into the int32 hash table in FTLLower
Branch(...,  #2, #3)

bb#2
ArrayifyToStructure(Cell:@A) // This modifies @A to have the its previous type union the type of some structure set.
Jump(#3)

bb#3
c: Add(Int32:@something, Int32:@A)
```

When the Add node does lowInt32() for @A, FTL lower used to just grab it
from the int32 hash table without filtering the AbstractValue. However,
the parent node is asking for a type check to happen, so we must inform
AI of this "type check" if we want to appease the assertion that all nodes
perform type checks for their edges that semantically perform type checks.
This patch makes it so we filter the AbstractValue in the lowXYZ even
if FTLLower proved the value must be XYZ.

* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compilePhi):
(JSC::FTL::DFG::LowerDFGToB3::simulatedTypeCheck):
(JSC::FTL::DFG::LowerDFGToB3::lowInt32):
(JSC::FTL::DFG::LowerDFGToB3::lowCell):
(JSC::FTL::DFG::LowerDFGToB3::lowBoolean):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@236824 268f45cc-cd09-0410-ab3c-d52691b4dbfc
pulkomandy pushed a commit that referenced this pull request Dec 5, 2018
LLIntSlowPath's llint_loop_osr and llint_replace should set the topCa…
ae0e6f0 
…llFrame.

https://bugs.webkit.org/show_bug.cgi?id=191579
<rdar://problem/45942472>

Reviewed by Saam Barati.

JSTests:

* stress/regress-191579.js: Added.

Source/JavaScriptCore:

Both of these functions do a lot of work.  It would be good for the topCallFrame
to be correct should we need to throw an exception.

For example, we've observed the following crash trace:

  * frame #0: WTFCrash() at Assertions.cpp:253
    frame #1: ...
    frame #2: JSC::StructureIDTable::get(this=0x00006040000162f0, structureID=1874583248) at StructureIDTable.h:129
    frame #3: JSC::VM::getStructure(this=0x0000604000016210, id=4022066896) at VM.h:705
    frame #4: JSC::JSCell::structure(this=0x00007ffeefbbde30, vm=0x0000604000016210) const at JSCellInlines.h:125
    frame #5: JSC::JSCell::classInfo(this=0x00007ffeefbbde30, vm=0x0000604000016210) const at JSCellInlines.h:335
    frame #6: JSC::JSCell::inherits(this=0x00007ffeefbbde30, vm=0x0000604000016210, info=0x0000000105eaf020) const at JSCellInlines.h:302
    frame #7: JSC::JSObject* JSC::jsCast<JSC::JSObject*, JSC::JSCell>(from=0x00007ffeefbbde30) at JSCast.h:36
    frame #8: JSC::asObject(cell=0x00007ffeefbbde30) at JSObject.h:1299
    frame #9: JSC::asObject(value=JSValue @ 0x00007ffeefbba380) at JSObject.h:1304
    frame #10: JSC::Register::object(this=0x00007ffeefbbdd58) const at JSObject.h:1514
    frame #11: JSC::ExecState::jsCallee(this=0x00007ffeefbbdd40) const at CallFrame.h:107
    frame #12: JSC::ExecState::isStackOverflowFrame(this=0x00007ffeefbbdd40) const at CallFrameInlines.h:36
    frame #13: JSC::StackVisitor::StackVisitor(this=0x00007ffeefbba860, startFrame=0x00007ffeefbbdd40, vm=0x0000631000000800) at StackVisitor.cpp:52
    frame #14: JSC::StackVisitor::StackVisitor(this=0x00007ffeefbba860, startFrame=0x00007ffeefbbdd40, vm=0x0000631000000800) at StackVisitor.cpp:41
    frame #15: void JSC::StackVisitor::visit<(JSC::StackVisitor::EmptyEntryFrameAction)0, JSC::Interpreter::getStackTrace(JSC::JSCell*, WTF::Vector<JSC::StackFrame, 0ul, WTF::CrashOnOverflow, 16ul>&, unsigned long, unsigned long)::$_3>(startFrame=0x00007ffeefbbdd40, vm=0x0000631000000800, functor=0x00007ffeefbbaa60)::$_3 const&) at StackVisitor.h:147
    frame #16: JSC::Interpreter::getStackTrace(this=0x0000602000005db0, owner=0x000062d00020cbe0, results=0x00006020000249d0, framesToSkip=0, maxStackSize=1) at Interpreter.cpp:437
    frame #17: JSC::getStackTrace(exec=0x000062d00002c048, vm=0x0000631000000800, obj=0x000062d00020cbe0, useCurrentFrame=true) at Error.cpp:170
    frame #18: JSC::ErrorInstance::finishCreation(this=0x000062d00020cbe0, exec=0x000062d00002c048, vm=0x0000631000000800, message=0x00007ffeefbbb800, useCurrentFrame=true) at ErrorInstance.cpp:119
    frame #19: JSC::ErrorInstance::create(exec=0x000062d00002c048, vm=0x0000631000000800, structure=0x000062d0000f5730, message=0x00007ffeefbbb800, appender=0x0000000000000000, type=TypeNothing, useCurrentFrame=true)(WTF::String const&, WTF::String const&, JSC::RuntimeType, JSC::ErrorInstance::SourceTextWhereErrorOccurred), JSC::RuntimeType, bool) at ErrorInstance.h:49
    frame #20: JSC::createRangeError(exec=0x000062d00002c048, globalObject=0x000062d00002c000, message=0x00007ffeefbbb800, appender=0x0000000000000000)(WTF::String const&, WTF::String const&, JSC::RuntimeType, JSC::ErrorInstance::SourceTextWhereErrorOccurred)) at Error.cpp:68
    frame #21: JSC::createRangeError(exec=0x000062d00002c048, globalObject=0x000062d00002c000, message=0x00007ffeefbbb800) at Error.cpp:316
    frame #22: JSC::createStackOverflowError(exec=0x000062d00002c048, globalObject=0x000062d00002c000) at ExceptionHelpers.cpp:77
    frame #23: JSC::createStackOverflowError(exec=0x000062d00002c048) at ExceptionHelpers.cpp:72
    frame #24: JSC::throwStackOverflowError(exec=0x000062d00002c048, scope=0x00007ffeefbbbaa0) at ExceptionHelpers.cpp:335
    frame #25: JSC::ProxyObject::getOwnPropertySlotCommon(this=0x000062d000200e40, exec=0x000062d00002c048, propertyName=PropertyName @ 0x00007ffeefbbba80, slot=0x00007ffeefbbc720) at ProxyObject.cpp:372
    frame #26: JSC::ProxyObject::getOwnPropertySlot(object=0x000062d000200e40, exec=0x000062d00002c048, propertyName=PropertyName @ 0x00007ffeefbbbd40, slot=0x00007ffeefbbc720) at ProxyObject.cpp:395
    frame #27: JSC::JSObject::getNonIndexPropertySlot(this=0x000062d000200e40, exec=0x000062d00002c048, propertyName=PropertyName @ 0x00007ffeefbbbea0, slot=0x00007ffeefbbc720) at JSObjectInlines.h:150
    frame #28: bool JSC::JSObject::getPropertySlot<false>(this=0x000062d000200e40, exec=0x000062d00002c048, propertyName=PropertyName @ 0x00007ffeefbbc320, slot=0x00007ffeefbbc720) at JSObject.h:1424
    frame #29: JSC::JSObject::calculatedClassName(object=0x000062d000200e40) at JSObject.cpp:535
    frame #30: JSC::Structure::toStructureShape(this=0x000062d000007410, value=JSValue @ 0x00007ffeefbbcae0, sawPolyProtoStructure=0x00007ffeefbbcf60) at Structure.cpp:1142
    frame #31: JSC::TypeProfilerLog::processLogEntries(this=0x000060400000a950, reason=0x00007ffeefbbd5c0) at TypeProfilerLog.cpp:89
    frame #32: JSC::JIT::doMainThreadPreparationBeforeCompile(this=0x0000619000034da0) at JIT.cpp:951
    frame #33: JSC::JITWorklist::Plan::Plan(this=0x0000619000034d80, codeBlock=0x000062d0001d88c0, loopOSREntryBytecodeOffset=0) at JITWorklist.cpp:43
    frame #34: JSC::JITWorklist::Plan::Plan(this=0x0000619000034d80, codeBlock=0x000062d0001d88c0, loopOSREntryBytecodeOffset=0) at JITWorklist.cpp:42
    frame #35: JSC::JITWorklist::compileLater(this=0x0000616000001b80, codeBlock=0x000062d0001d88c0, loopOSREntryBytecodeOffset=0) at JITWorklist.cpp:256
    frame #36: JSC::LLInt::jitCompileAndSetHeuristics(codeBlock=0x000062d0001d88c0, exec=0x00007ffeefbbde30, loopOSREntryBytecodeOffset=0) at LLIntSlowPaths.cpp:391
    frame #37: llint_replace(exec=0x00007ffeefbbde30, pc=0x00006040000161ba) at LLIntSlowPaths.cpp:516
    frame #38: llint_entry at LowLevelInterpreter64.asm:98
    frame #39: vmEntryToJavaScript at LowLevelInterpreter64.asm:296
    ...

This crash occurred because StackVisitor was seeing an invalid topCallFrame while
trying to capture the Error stack while throwing a StackOverflowError below
llint_replace.  While in this specific example, it is questionable whether we
should be executing JS code below TypeProfilerLog::processLogEntries(), it is
correct to have set the topCallFrame in llint_replace.  We do this by calling
LLINT_BEGIN_NO_SET_PC() at the top of llint_replace.

We also do the same for llint_osr.
        
Note: both of these LLInt slow path functions are called with a fully initialized
CallFrame.  Hence, there's no issue with setting topCallFrame to their CallFrames
for these functions.

* llint/LLIntSlowPaths.cpp:
(JSC::LLInt::LLINT_SLOW_PATH_DECL):



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@238141 268f45cc-cd09-0410-ab3c-d52691b4dbfc
pulkomandy pushed a commit that referenced this pull request May 4, 2019
[JSC] LLIntEntryPoint creates same DirectJITCode for all functions
49f5dbf 
https://bugs.webkit.org/show_bug.cgi?id=194648

Reviewed by Keith Miller.

JSTests:

* microbenchmarks/generate-multiple-llint-entrypoints.js: Added.

Source/JavaScriptCore:

1. Making LLIntThunks singleton.

Motivation: Former implementation has one LLIntThunk per type per VM.
However, the generated code for every kind of thunk is essentially the
same and we end up wasting memory (right now jitAllocationGranule = 32 bytes)
when we have 2 or more VM instantiated. Turn these thunks into
singleton will avoid such wasting.

Tradeoff: This change comes with a price, because we will keep thunks
allocated even when there is no VM instantiated. Considering WebCore use case,
the situation of having no VM instantiated is uncommon, since once a
VM is created through `commomVM()`, it will never be destroyed. Given
that, this change does not impact the overall memory comsumption of
WebCore/JSC. It also doesn't impact memory footprint, since thunks are
generated lazily (see results below).

Since we are keeping a static `MacroAssemblerCodeRef<JITThunkPtrTag>`,
we have the assurance that JITed code will never be deallocated,
given it is being pointed by `RefPtr<ExecutableMemoryHandle> m_executableMemory`.
To understand why we decided to make LLIntThunks singleton instead of
removing them, please see the comment on `llint/LLIntThunks.cpp`.

2. Making all LLIntEntrypoints singleton

Motivation: With singleton LLIntThunks, we also can have singleton
DirectJITCodes and NativeJITCodes for each LLIntEntrypoint type and
avoid multiple allocations of objects with the same content.

Tradeoff: As explained before, once we allocate an entrypoint, it
will be alive until the program exits. However, the gains we can
achieve in some use cases justifies such allocations.

As DirectJITCode and NativeJITCode are ThreadSafeRefCounted and we are using
`codeBlock->setJITCode(makeRef(*jitCode))`, their reference counter
will never be less than 1.

3. Memory usage analysis

This change reduces memory usage on stress/generate-multiple-llint-entrypoints.js
by 2% and is neutral on JetStream 2. Following results were generated
running each benchmark 6 times and using 95% Student's t distribution
confidence interval.

microbenchmarks/generate-multiple-llint-entrypoints.js (Changes uses less memory):
    Mean of memory peak on ToT: 122576896 bytes (confidence interval: 67747.2316)
    Mean of memory peak on Changes: 119248213.33 bytes (confidence interval: 50251.2718)

JetStream2 (Neutral):
    Mean of memory peak on ToT: 5442742272 bytes (confidence interval: 134381565.9117)
    Mean of memory peak on Changes: 5384949760 bytes (confidence interval: 158413904.8352)

4. Performance Analysis

This change is performance neutral on JetStream 2 and Speedometer 2.
See results below.:

JetStream 2 (Neutral):
    Mean of score on ToT: 139.58 (confidence interval: 2.44)
    Mean of score on Changes: 141.46 (confidence interval: 4.24)

Speedometer run #1
   ToT: 110 +- 2.9
   Changes: 110 +- 1.8

Speedometer run #2
   ToT: 110 +- 1.6
   Changes: 108 +- 2.3

Speedometer run #3
   ToT: 110 +- 3.0
   Changes: 110 +- 1.4

* jit/JSInterfaceJIT.h:
(JSC::JSInterfaceJIT::JSInterfaceJIT):
* llint/LLIntEntrypoint.cpp:

Here we are changing the usage or DirectJITCode by NativeJITCode on cases
where there is no difference from address of calls with and without
ArithCheck.

(JSC::LLInt::setFunctionEntrypoint):
(JSC::LLInt::setEvalEntrypoint):
(JSC::LLInt::setProgramEntrypoint):
(JSC::LLInt::setModuleProgramEntrypoint):
(JSC::LLInt::setEntrypoint):
* llint/LLIntEntrypoint.h:
* llint/LLIntThunks.cpp:
(JSC::LLInt::generateThunkWithJumpTo):
(JSC::LLInt::functionForCallEntryThunk):
(JSC::LLInt::functionForConstructEntryThunk):
(JSC::LLInt::functionForCallArityCheckThunk):
(JSC::LLInt::functionForConstructArityCheckThunk):
(JSC::LLInt::evalEntryThunk):
(JSC::LLInt::programEntryThunk):
(JSC::LLInt::moduleProgramEntryThunk):
(JSC::LLInt::functionForCallEntryThunkGenerator): Deleted.
(JSC::LLInt::functionForConstructEntryThunkGenerator): Deleted.
(JSC::LLInt::functionForCallArityCheckThunkGenerator): Deleted.
(JSC::LLInt::functionForConstructArityCheckThunkGenerator): Deleted.
(JSC::LLInt::evalEntryThunkGenerator): Deleted.
(JSC::LLInt::programEntryThunkGenerator): Deleted.
(JSC::LLInt::moduleProgramEntryThunkGenerator): Deleted.
* llint/LLIntThunks.h:
* runtime/ScriptExecutable.cpp:
(JSC::setupLLInt):
(JSC::ScriptExecutable::prepareForExecutionImpl):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@243136 268f45cc-cd09-0410-ab3c-d52691b4dbfc
pulkomandy pushed a commit that referenced this pull request May 18, 2019
AbstractValue can represent more than int52
4a459f1 
https://bugs.webkit.org/show_bug.cgi?id=197118
<rdar://problem/49969960>

Reviewed by Michael Saboff.

JSTests:

* stress/abstract-value-can-include-int52.js: Added.
(foo):
(index.index.8.index.60.index.65.index.1234.index.1234.parseInt.string_appeared_here.String.fromCharCode):

Source/JavaScriptCore:

Let's analyze this control flow diamond:

#0
branch #1, #2

#1:
PutStack(JSValue, loc42)
Jump #3

#2:
PutStack(Int52, loc42)
Jump #3

#3:
...

Our abstract value for loc42 at the head of #3 will contain an abstract
value that us the union of Int52 with other things. Obviously in the
above program, a GetStack for loc42 would be inavlid, since it might
be loading either JSValue or Int52. However, the abstract interpreter
just tracks what the value could be, and it could be Int52 or JSValue.

When I did the Int52 refactoring, I expected such things to never happen,
but it turns out it does. We should just allow for this instead of asserting
against it since it's valid IR to do the above.

* bytecode/SpeculatedType.cpp:
(JSC::dumpSpeculation):
* dfg/DFGAbstractValue.cpp:
(JSC::DFG::AbstractValue::checkConsistency const):
* dfg/DFGAbstractValue.h:
(JSC::DFG::AbstractValue::validateTypeAcceptingBoxedInt52 const):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@244480 268f45cc-cd09-0410-ab3c-d52691b4dbfc
pulkomandy pushed a commit that referenced this pull request Dec 8, 2019
[JSC] ObjectAllocationSinkingPhase wrongly deals with always-taken br…
89ed7dd 
…anches during interpretation

https://bugs.webkit.org/show_bug.cgi?id=198650

Reviewed by Saam Barati.

JSTests:

* stress/object-allocation-sinking-interpretation-can-interpret-edges-that-can-be-proven-unreachable-in-ai.js:
(main.v0):
(main):

Source/JavaScriptCore:

Object Allocation Sinking phase has a lightweight abstract interpreter which interprets DFG nodes related to allocations and properties.
This interpreter is lightweight since it does not track abstract values and conditions as deeply as AI does. It can happen that this
interpreter interpret the control-flow edge that AI proved that is never taken.
AI already knows some control-flow edges are never taken, and based on this information, AI can remove CheckStructure nodes. But
ObjectAllocationSinking phase can trace this never-taken edges and propagate structure information that contradicts to the analysis
done in ObjectAllocationSinking.

Let's see the example.

    BB#0
        35: NewObject([%AM:Object])
        ...
        47: Branch(ConstantTrue, T:#1, F:#2)

    BB#1 // This basic block is never taken due to @47's jump.
        ...
        71: PutByOffset(@35, @66, id2{a}, 0, W:NamedProperties(2))
        72: PutStructure(@35, %AM:Object -> %Dx:Object, ID:60066)
        ...
        XX: Jump(#2)

    BB#2
        ...
        92: CheckStructure(@35, [%Dx:Object])
        93: PutByOffset(@35, @35, id2{a}, 0, W:NamedProperties(2))
        ...

AI removes @92 because AI knows BB#0 only takes BB#1 branch. @35's Structure is always %Dx so @92 is redundant.
AI proved that @71 and @72 are always executed while BB#0 -> BB#2 edge is never taken so that @35 object's structure is proven at @92.
After AI removes @92, ObjectAllocationSinking starts looking into this graph.

    BB#0
        35: NewObject([%AM:Object])
        ...
        47: Branch(ConstantTrue, T:#1, F:#2)

    BB#1 // This basic block is never taken due to @47's jump.
        ...
        71: PutByOffset(@35, @66, id2{a}, 0, W:NamedProperties(2))
        72: PutStructure(@35, %AM:Object -> %Dx:Object, ID:60066)
        ...
        XX: Jump(#2)

    BB#2
        ...
        93: PutByOffset(@35, @35, id2{a}, 0, W:NamedProperties(2))
        ...
        YY: Jump(#3)

    BB#3
        ...
        ZZ: <HERE> want to materialize @35's sunk object.

Since AI does not change the @47 Branch to Jump (it is OK anyway), BB#0 -> BB#2 edge remains and ObjectAllocationSinking phase propagates information in
BB#0's %AM structure information to BB#2. ObjectAllocationSinking phase converts @35 to PhantomNewObject, removes PutByOffset and PutStructure, and
insert MaterializeNewObject in @zz. At this point, ObjectAllocationSinking lightweight interpreter gets two structures while AI gets one: @35's original
one (%AM) and @72's replaced one (%Dx). Since AI already proved @zz only gets %Dx, AI removed @92 CheckStructure. But this is not known to ObjectAllocationSinking
phase's interpretation. So when creating recovery data, MultiPutByOffset includes two structures, %AM and %Dx. This is OK since MultiPutByOffset takes
conservative set of structures and performs switching. But the problem here is that %AM's id2{a} offset is -1 since %AM does not have such a property.
So when creating MultiPutByOffset in ObjectAllocationSinking, we accidentally create MultiPutByOffset with -1 offset data, and lowering phase hits the debug
assertion.

    187: MultiPutByOffset(@138, @138, id2{a}, <Replace: [%AM:Object], offset = -1, >, <Replace: [%Dx:Object], offset = 0, >)

This bug is harmless since %AM structure comparison never meets at runtime. But we are not considering the case including `-1` offset property in MultiPutByOffset data.
In this patch, we just filter out apparently wrong structures when creating MultiPutByOffset in ObjectAllocationSinking. This is OK since it never comes at runtime.

* dfg/DFGObjectAllocationSinkingPhase.cpp:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@249306 268f45cc-cd09-0410-ab3c-d52691b4dbfc
pulkomandy pushed a commit that referenced this pull request Jan 12, 2020
[LFC][IFC] Keep the LineBreaker object around until after the line is…
ad625a0 
… closed.

https://bugs.webkit.org/show_bug.cgi?id=205613
<rdar://problem/58222870>

Reviewed by Antti Koivisto.

In order to be able to point back to an earlier line wrap opportunity on the line e.g.
<div style="white-space: pre"><span style="white-space: normal">earlier_wrap opportunities</span> <span>can't_wrap_this content</span></div>
the LineBreaker class needs more context.
Currently (taking the example above), if the available space runs out somewhere around the second <span> we would just simply
overflow the line since the overflowing content has a style saying "do not wrap".
However the line has multiple earlier wrap opportunities inside the first <span>.
Since we construct a LineBreaker object for each continuous run

1. [container start][earlier_wrap]
2. [ ]
3. [opportunities][container end]
4. [ ]
5. [container start][can't_wrap_this]
6. [ ]
7. [content][container end]

the LineBreaker does not have enough context to point back to the last line wrap opportunity (after run #3).

This patch is in preparation for supporting such content.
1. Rename couple of functions and structs
2. Move the LineBreaker construction to LineLayoutContext::layoutLine so that we can keep it around for multiple set of runs.

* layout/inlineformatting/InlineLineBreaker.cpp:
(WebCore::Layout::isContentWrappingAllowed):
(WebCore::Layout::isContentSplitAllowed):
(WebCore::Layout::LineBreaker::isTextContentWrappingAllowed const):
(WebCore::Layout::LineBreaker::shouldKeepEndOfLineWhitespace const):
(WebCore::Layout::LineBreaker::shouldWrapInlineContent):
(WebCore::Layout::isTextContentWrappingAllowed): Deleted.
(WebCore::Layout::shouldKeepEndOfLineWhitespace): Deleted.
(WebCore::Layout::LineBreaker::breakingContextForInlineContent): Deleted.
* layout/inlineformatting/InlineLineBreaker.h:
(WebCore::Layout::LineBreaker::setHyphenationDisabled):
* layout/inlineformatting/LineLayoutContext.cpp:
(WebCore::Layout::isLineConsideredEmpty):
(WebCore::Layout::LineLayoutContext::layoutLine):
(WebCore::Layout::LineLayoutContext::checkForLineWrapAndCommit):
(WebCore::Layout::LineLayoutContext::commitContent):
(WebCore::Layout::LineLayoutContext::placeInlineContentOnCurrentLine): Deleted.
* layout/inlineformatting/LineLayoutContext.h:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@253924 268f45cc-cd09-0410-ab3c-d52691b4dbfc
pulkomandy pushed a commit that referenced this pull request Jan 26, 2020
JSTests:
df3b07e 
Object allocation sinking is missing PutHint for allocations unreachable in the graph
https://bugs.webkit.org/show_bug.cgi?id=203799
<rdar://problem/56852162>

Reviewed by Saam Barati.

* stress/allocation-sinking-puthint-control-flow-2.js: Added.
(f.handler.construct):
(f):

Source/JavaScriptCore:
Object allocation sinking is missing PutHint for sunken allocations
https://bugs.webkit.org/show_bug.cgi?id=203799
<rdar://problem/56852162>

Reviewed by Saam Barati.

Consider the following graph:

    Block #0:
      1: PhantomCreateActivation()
      2: PhantomNewFunction()
      PutHint(@2, @1, FunctionActivationPLoc)
      Branch(#1, #2)

    Block #1:
      3: MaterializeCreateActivation()
      PutHint(@2, @3, FunctionActivationPLoc)
      Upsilon(@3, ^5)
      Jump(#3)

    Block #2:
      4: MaterializeCreateActivation()
      PutHint(@2, @4, FunctionActivationPLoc)
      Upsilon(@4, ^5)
      Jump(#3)

    Block #3:
      5: Phi()
      ExitOK()

On Block #3, we need to emit a PutHint after the Phi, since we might exit after it. However,
object allocation sinking skipped this Phi because it was checking whether the base of the
location that caused us to create this Phi (@2) was live, but it's dead in the graph (there
are no pointers to it).  The issue is that, even though there are no pointers to the base, the
location `PromotedHeapLocation(@2, FunctionActivationPLoc)` is still live, so we should PutHint
to it. We fix it by checking for liveness of the location rather than its base.

* dfg/DFGObjectAllocationSinkingPhase.cpp:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@254725 268f45cc-cd09-0410-ab3c-d52691b4dbfc
pulkomandy pushed a commit that referenced this pull request Feb 6, 2020
JSTests:
83edfb3 
Object allocation sinking is missing PutHint for allocations unreachable in the graph
https://bugs.webkit.org/show_bug.cgi?id=203799
<rdar://problem/56852162>

Reviewed by Saam Barati.

* stress/allocation-sinking-puthint-control-flow-2.js: Added.
(f.handler.construct):
(f):

Source/JavaScriptCore:
Object allocation sinking is missing PutHint for sunken allocations
https://bugs.webkit.org/show_bug.cgi?id=203799
<rdar://problem/56852162>

Reviewed by Saam Barati.

Consider the following graph:

    Block #0:
      1: PhantomCreateActivation()
      2: PhantomNewFunction()
      PutHint(@2, @1, FunctionActivationPLoc)
      Branch(#1, #2)

    Block #1:
      3: MaterializeCreateActivation()
      PutHint(@2, @3, FunctionActivationPLoc)
      Upsilon(@3, ^5)
      Jump(#3)

    Block #2:
      4: MaterializeCreateActivation()
      PutHint(@2, @4, FunctionActivationPLoc)
      Upsilon(@4, ^5)
      Jump(#3)

    Block #3:
      5: Phi()
      ExitOK()

On Block #3, we need to emit a PutHint after the Phi, since we might exit after it. However,
object allocation sinking skipped this Phi because it was checking whether the base of the
location that caused us to create this Phi (@2) was live, but it's dead in the graph (there
are no pointers to it).  The issue is that, even though there are no pointers to the base, the
location `PromotedHeapLocation(@2, FunctionActivationPLoc)` is still live, so we should PutHint
to it. We fix it by checking for liveness of the location rather than its base.

* dfg/DFGObjectAllocationSinkingPhase.cpp:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@254866 268f45cc-cd09-0410-ab3c-d52691b4dbfc
pulkomandy pushed a commit that referenced this pull request Aug 10, 2020
[LFC][TFC] Add support for percentage min/max-width
d42669f 
https://bugs.webkit.org/show_bug.cgi?id=213436

Reviewed by Antti Koivisto.

Source/WebCore:

1. The table generates a principal block container box called the table wrapper box that contains the table box itself and any caption boxes.
2. The table wrapper box establishes a block formatting context, and the table box establishes a table formatting context.
3. The computed values of properties 'position', 'float', 'margin-*', 'top', 'right', 'bottom', and 'left' on
   the table element are used on the table wrapper box and not the table box; all other values of non-inheritable
   properties are used on the table box and not the table wrapper box.
4. In a block formatting context, each box's left outer edge touches the left edge of the containing block.
   This is true even in the presence of floats, unless the box establishes a new block formatting context (in which case the
   box itself may become narrower due to the floats)

Now consider the following case:
<div style="display: block; width: 500px;">
    <div style="float: left; width: 100px;"></div>
    <div style="display: table; width: 10%;"></div>
</div>
1. We create a table wrapper box to wrap the "display: table" block level box (#1).
2. The table wrapper box's width property is set to auto (#3).
3. Since it establishes a new block formatting context, the available horizontal space gets shrunk by the float (#4)
4. The table wrapper box's used width computes to 500px - 100px -> 400px;

Now we are inside the BFC established by the table wrapper box and try to resolve the table's width -> %10.
According to the normal BFC rules, it should compute to 10% of the containing block's logical width: 400px -> 40px.
However in practice it computes to 50px (10% of 500px).

Similar setup with non-table content would resolve the inner block level box's width to 40px;
<div style="display: block; width: 500px">
    <div style="float: left; width: 100px;"></div>
    <div style="display: block; overflow: hidden;">
        <div style="display: block; width: 10%"></div>
    </div>
</div>
This needs clarification.

Test: fast/layoutformattingcontext/float-avoider-available-horizontal-space3.html

* layout/FormattingContext.h:
(WebCore::Layout::FormattingContext::isTableWrapperBlockFormattingContext const):
* layout/blockformatting/BlockFormattingContext.cpp:
(WebCore::Layout::BlockFormattingContext::layoutInFlowContent):
* layout/blockformatting/tablewrapper/TableWrapperBlockFormattingContext.cpp:
(WebCore::Layout::TableWrapperBlockFormattingContext::computeWidthAndMarginForTableBox):
* layout/blockformatting/tablewrapper/TableWrapperBlockFormattingContext.h:

LayoutTests:

* fast/layoutformattingcontext/float-avoider-available-horizontal-space3-expected.html: Added.
* fast/layoutformattingcontext/float-avoider-available-horizontal-space3.html: Added.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@263327 268f45cc-cd09-0410-ab3c-d52691b4dbfc
pulkomandy pushed a commit that referenced this pull request Aug 18, 2020
OSRAvailabilityAnalysis shouldn't mark GetStack nodes directly as val…
f909e16 
…id places for recovery

https://bugs.webkit.org/show_bug.cgi?id=215434

Reviewed by Saam Barati.

JSTests:

* stress/for-of-post-sinking-osr-availability.js: Added.
(foo):

Source/JavaScriptCore:

It's somewhat subtle why we cannot use the node for the GetStack
itself in the Availability's node field. The reason is that if we
did we would need to make any phase that converts nodes to
GetStack availability aware. For instance, a place where this
could come up is in constant folding if you had a graph like the
following, which could arise from PutStack sinking:

BB#1:
@1: NewObject()
@2: MovHint(@1, inline-arg1)
@3: Jump(#2, #3)

BB#2:
@4: PutStack(@1, inline-arg1)
@5: GetMyArgumentByVal(inline-arg1)
@6: Jump(#3)

BB#3:
@7: InvalidationPoint()

If constant folding converts @5 to a GetStack then at @7
inline-arg1 won't be available since at the end of BB#1 our
availability is (@1, DeadFlush) and (@5,
FlushedAt(inline-arg1)). When that gets merged at BB#3 then the
availability will be (nullptr, ConflictingFlush).

This patch also makes validation check that availability is sane
at each pontential exit site if
Options::validateFTLOSRExitLiveness() is set. Since this is
actually a Phase we also need to make sure that we don't infinite
loop, so there is now a m_isValidating field on m_graph. The
validateOSRExitAvailability phase is also careful not to modify
the Graph, in order to avoid masking bugs when validating.

In a followup patch I intend to look into why MovHint elimination
will convert:

@2: MovHint(@0, loc1, bc#1, ExitInvalid)
@3: KillStack(loc1, bc#2, ExitValid)
@4: MovHint(@1 loc1, bc#2, ExitInvalid)

into

@2: ZombieHint(@0, loc1, bc#1, ExitInvalid)
@3: KillStack(loc1, bc#2, ExitValid)
@4: MovHint(@1 loc1, bc#2, ExitInvalid)

when loc1 is live in the bytecode at bc#2. But for now, the
validation rule works around this by only checking when mayExit
and the nodes exitOK agree exiting is possible.

* dfg/DFGGraph.h:
* dfg/DFGOSRAvailabilityAnalysisPhase.cpp:
(JSC::DFG::OSRAvailabilityAnalysisPhase::OSRAvailabilityAnalysisPhase):
(JSC::DFG::OSRAvailabilityAnalysisPhase::run):
(JSC::DFG::performOSRAvailabilityAnalysis):
(JSC::DFG::validateOSRExitAvailability):
(JSC::DFG::LocalOSRAvailabilityCalculator::executeNode):
* dfg/DFGOSRAvailabilityAnalysisPhase.h:
* dfg/DFGPhase.h:
(JSC::DFG::runPhase):
* dfg/DFGValidate.cpp:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@265685 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
2 participants
@vchrombie @pulkomandy