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

a proposal for correcting 4797 #6844

Closed
wants to merge 7 commits into from
Closed

a proposal for correcting 4797 #6844

wants to merge 7 commits into from

Conversation

archibaldhaddock
Copy link

Hello,

This one is for correcting issue #4797.
That's my first pull request, let's see if I 'll survive...

Well, dont think so ...."archibaldhaddock wants to merge 7 commits into master from HHVM-3.12"
Never in my life !
Well, I'll see tomorow if I can do something wih my git diff and test file...

kodafb and others added 7 commits February 10, 2016 12:10
Summary:
Add snapshot support for APC priming, to load faster and with less memory than current shared library format.

The new format is auto-detected when passed to the existing PrimeLibrary flag (it can even keep the .so extension).

To "upgrade" an existing .so to the new format, there is a new flag that just writes the snapshot to a file and then exits hhvm (without starting any servers).

Reviewed By: binliu19

Differential Revision: D2826183

fb-gh-sync-id: 5f49d96
Reviewed By: emiraga

Differential Revision: D2845107

fb-gh-sync-id: 9364b82
Summary:
The implemention of in() in hhbbc/interp.cpp for the CGetNQuiet
bytecode should be identical to that of CGetN. The intention was that we could
re-use the same implemention by just calling the CGetN implementation. Since the
CGetN implementation did not actually refer to anything in the opcode, we could
obtain the proper overloading behavior by just casting a null to a
CGetN. Unfortunately, a typo meant that we casted to CGetNQuiet instead, meaning
we just recursively call ourself infinitely.

Rather than just fix the typo, avoid the hackery of casting a nullptr by
factoring out the CGetN implementation into a separate function and having both
CGetN and CGetNQuiet call that. It also turns out that the implementations
shouldn't be quite identical, they need a template parameter to determine which
CGetL equivalent they should be optimized to.

Also add a test case to actually exercise these different "quiet" versions of
CGet.

Reviewed By: markw65

Differential Revision: D2850127

fb-gh-sync-id: bf9142e
Summary:
If anything threw while it was loading the unit, the unit would be
forever marked as not existing.

Originally, nothing should have thrown, but we added surprise checks
to the unserialize code, so timeouts, and ooms could end up throwing.

This only affects RepoAuthoritative mode; in sandbox mode, we'll just
try again if we find an entry with a null unit.

Reviewed By: swtaarrs

Differential Revision: D2867957

fb-gh-sync-id: 8eddfa5
Summary:
When hhvm was built in fbsource, we would get a version string looking
like "gfbcode: <sha1>" rather than "g<sha1>".

This broke various tools, and at least one flib test.

Reviewed By: mxw

Differential Revision: D2866405

fb-gh-sync-id: e66ec77
@facebook-github-bot
Copy link
Contributor

This pull request has been imported into Phabricator, and discussion and review of the diff will take place at https://reviews.facebook.net/D54441

@archibaldhaddock archibaldhaddock changed the title Hhvm 3.12 correcting 4797 Feb 19, 2016
@archibaldhaddock archibaldhaddock changed the title correcting 4797 co Feb 19, 2016
@archibaldhaddock archibaldhaddock changed the title co corr Feb 19, 2016
@archibaldhaddock archibaldhaddock changed the title corr correcting 4797 Feb 19, 2016
@archibaldhaddock archibaldhaddock changed the title correcting 4797 a proposal for correcting 4797 Feb 19, 2016
@JoelMarcey
Copy link
Contributor

There are too many commits associated with this pull request and you are trying to patch stuff from a release branch into master, which is also odd.

I am going to close this, and you can try again if you want.

Normally you would fork your repo from master make your pull request from there.

@JoelMarcey JoelMarcey closed this Feb 19, 2016
@archibaldhaddock
Copy link
Author

Ok, thanks Joel !
I'll try again tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants