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.

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

Redefer function bodies #1585

Merged
merged 1 commit into from
Nov 11, 2016
Merged

Redefer function bodies #1585

merged 1 commit into from
Nov 11, 2016

Conversation

pleath
Copy link
Contributor

@pleath pleath commented Sep 15, 2016

Redefer function bodies that are not currently being executed and are eligible for deferred parsing (e.g., not arrow functions, not functions-in-block). Define a 'force' mode in which all eligible functions are redeferred on GC, as well as a 'stress' mode in which all candidates are redeferred on each stack probe.

@pleath
Copy link
Contributor Author

pleath commented Oct 22, 2016

Merged with OOP JIT. That was fun. :)

@pleath
Copy link
Contributor Author

pleath commented Oct 22, 2016

Fully synched.

@@ -1763,7 +1763,7 @@ void BailOutRecord::ScheduleFunctionCodeGen(Js::ScriptFunction * function, Js::S
bailOutRecordNotConst->bailOutCount++;

Js::FunctionEntryPointInfo *entryPointInfo = function->GetFunctionEntryPointInfo();
uint8 callsCount = entryPointInfo->callsCount;
uint32 callsCount = entryPointInfo->callsCount;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @LouisLaf mentioned in #1757, we should have callsCount be MIN of entryPointInfo->callsCount and 255

entryPointInfo->totalJittedLoopIterations = UINT8_MAX;
}
uint8 totalJittedLoopIterations = (uint8)entryPointInfo->totalJittedLoopIterations;
uint32 totalJittedLoopIterations = (uint8)entryPointInfo->totalJittedLoopIterations;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should retain the previous rejit behaviour in keeping with @LouisLaf 's suggestion in #1757

@pleath pleath force-pushed the redefer branch 2 times, most recently from 189c0f3 to 0826835 Compare November 8, 2016 01:12
@pleath
Copy link
Contributor Author

pleath commented Nov 8, 2016

Reverted to treating call counts as saturating uint8's in bailout code. (Thanks, @rajatd.)

uint gcSinceLastRedeferral;
uint gcSinceCallCountsCollected;

static const uint InitialRedeferralDelay = 5;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InitialRedeferralDelay [](start = 22, length = 22)

should we have these as configurable values so we can tune redeferral in the future if need be?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea.

@@ -2062,19 +2080,6 @@ ThreadContext::ExecuteRecyclerCollectionFunction(Recycler * recycler, Collection

BOOL ret = FALSE;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RedeferFunctionBodies [](start = 23, length = 21)

Correct me if I'm wrong, but I think we should also pass the result of this->GetRedeferralalCollectionInterval() to ScriptContext::RedeferFunctionBodies. Right now, the result of ThreadContext::GetRedeferralInactiveThreshold is being used in FunctionBody::DoRedeferFunction to determine if a function has remained inactive for long enough that it could be redeferred.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining offline. No action needed for this comment.

uint inactiveCount;
auto fn = [&](){ inactiveCount = 0xFFFFFFFF; };
inactiveCount = UInt32Math::Mul(this->GetInactiveCount(), this->GetCompileCount(), fn);
if (inactiveCount < inactiveThreshold)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inactiveCount < inactiveThreshold [](start = 16, length = 33)

The idea here is that if a function was parsed after having been redeferred, make it less likely to be deferred again, right? If so, shouldn't we multiply the threshold by the compileCount?

(Attributes)(this->GetAttributes() & ~(Attributes::DeferredDeserialize | Attributes::DeferredParse)),
Js::FunctionBody::FunctionBodyFlags::Flags_HasNoExplicitReturnValue
#ifdef PERF_COUNTERS
, false /* is function from deferred deserialized proxy */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this assert no longer valid? If so, can we just remove it?

Assert(simpleJitLimit == 0 ? callCount == 0 : simpleJitLimit > callCount);
return callCount == 0 ?
static_cast<uint16>(simpleJitLimit) :
static_cast<uint16>(simpleJitLimit) - callCount - 1;
static_cast<uint16>(simpleJitLimit) - static_cast<uint16>(callCount) - 1;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just a bug fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just silencing any possible complaint about overflow.

else if (!canAllocInPreReservedHeapPageSegment && isAnyJittedCode)
{
*isAllJITCodeInPreReservedRegion = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not part of your change, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's a real issue that my change exposed, probably by causing more re-jitting.

@rajatd
Copy link
Contributor

rajatd commented Nov 10, 2016

LGTM apart from the comments.

@pleath pleath changed the title Redefer function bodies (experimental) Redefer function bodies Nov 10, 2016
@pleath
Copy link
Contributor Author

pleath commented Nov 10, 2016

@dotnet-bot please test Windows x86_release

@pleath pleath force-pushed the redefer branch 2 times, most recently from b5b2104 to 223b4a9 Compare November 11, 2016 01:28
eligible for deferred parsing (e.g., not arrow functions, not
functions-in-block). Define
a force mode in which all eligible functions are redeferred on GC, as well
as a 'stress' mode in which all candidates are redeferred on each stack
probe.
@chakrabot chakrabot merged commit a384d2e into chakra-core:master Nov 11, 2016
chakrabot pushed a commit that referenced this pull request Nov 11, 2016
Merge pull request #1585 from pleath:redefer

Redefer function bodies that are not currently being executed and are eligible for deferred parsing (e.g., not arrow functions, not functions-in-block). Define a 'force' mode in which all eligible functions are redeferred on GC, as well as a 'stress' mode in which all candidates are redeferred on each stack probe.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants