Skip to content

Use optimised forms of class release functions where possible #9673

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+29 −0
Open

Use optimised forms of class release functions where possible #9673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions hphp/runtime/base/object-data.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,10 @@ void ObjectData::release(ObjectData* obj, const Class* cls) noexcept {
AARCH64_WALKABLE_FRAME();
}

void ObjectData::release2(ObjectData* obj) noexcept {
release(obj, obj->m_cls.get());
}

///////////////////////////////////////////////////////////////////////////////
// class info

Expand Down
6 changes: 6 additions & 0 deletions hphp/runtime/base/object-data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,12 @@ struct ObjectData : Countable, type_scan::MarkCollectable<ObjectData> {
*/
static void release(ObjectData* obj, const Class* cls) noexcept;

/*
* An optimised version of the above function when the class can be loaded
* from the object.
*/
static void release2(ObjectData* obj) noexcept;

Class* getVMClass() const;
StrNR getClassName() const;

Expand Down
6 changes: 6 additions & 0 deletions hphp/runtime/vm/class.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5260,6 +5260,12 @@ std::vector<Class*> prioritySerializeClasses() {
return ret;
}

Optional<OptObjReleaseFunc> Class::optReleaseFunc() const {
if (m_releaseFunc == ObjectData::release)
return ObjectData::release2;
Copy link
Contributor

@mxw mxw Nov 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

unbraced multiline conditionals have always been a functional style prohibition. cf. "goto fail" for the reason why. prefer to either fit it on a single line or add braces. (i mention this bc i've noticed it in other PR's you've made recently, which i've seen out of idle curiosity.)

additionally, this doesn't need to be a member function of Class. m_releaseFunc is exposed as a const accessor by releaseFunc(), so you can and should just do this work at the one single callsite instead of polluting Class.

Copy link
Author

@david-arm david-arm Nov 19, 2025

Choose a reason for hiding this comment

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

OK thanks for letting me know! Happy to update that patch.

Copy link
Contributor

@mxw mxw Nov 19, 2025

Choose a reason for hiding this comment

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

oh wow, do comments not all commit transactionally with a "review"? clearly i have never used github PRs before in my life (accurate).

return std::nullopt;
}

///////////////////////////////////////////////////////////////////////////////

namespace {
Expand Down
10 changes: 10 additions & 0 deletions hphp/runtime/vm/class.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ using ClassPtr = AtomicSharedPackedPtr<Class>;
// Since native instance dtors can be release functions, they have to have
// compatible signatures.
using ObjReleaseFunc = BuiltinDtorFunction;
using OptObjReleaseFunc = SmallPtr<void(ObjectData*)>;

using ObjectProps = std::conditional<tv_layout::stores_unaligned_typed_values, tv_layout::UnalignedTVLayout, tv_layout::Tv7Up>::type;

Expand Down Expand Up @@ -825,6 +826,15 @@ struct Class : AtomicCountable {

ObjReleaseFunc releaseFunc() const;

/////////////////////////////////////////////////////////////////////////////
// Optimized Object release.
//
// Similar to the above, except only the object needs passing to the release
// function. The class is obtained directly from the object itself. If an
// optimized version does not exist, return std::nullopt.

Optional<OptObjReleaseFunc> optReleaseFunc() const;

/////////////////////////////////////////////////////////////////////////////
// Property metadata. [const]
//
Expand Down
3 changes: 3 additions & 0 deletions hphp/runtime/vm/jit/irlower-refcount.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,9 @@ CallSpec makeDtorCall(Vout& v, Type ty, Vloc loc, ArgGroup& args) {
// a builtin, as only builtins can override release method and builtins
// never subclass non-builtins.
if (!isInterface(cls) && !cls->isBuiltin()) {
if (auto rf = cls->optReleaseFunc()) {
return CallSpec::direct(rf->get());
}
args.reg(emitLdObjClass(v, loc.reg(0), v.makeReg()));
return CallSpec::direct(cls->releaseFunc().get());
}
Expand Down