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

Preliminary C++ proxy refactoring #1754

Merged
merged 5 commits into from
Feb 5, 2024
Merged

Preliminary C++ proxy refactoring #1754

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
27dddc7
Initial refactory of C++ proxies
bernardnormier Feb 4, 2024
e30b6e0
Make default constructor protected
bernardnormier Feb 4, 2024
4885e2d
Cleanup
bernardnormier Feb 4, 2024
d096138
Fix build failure
bernardnormier Feb 4, 2024
83c4ccf
Merge remote-tracking branch 'origin/main' into proxy1
bernardnormier Feb 5, 2024
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
3 changes: 1 addition & 2 deletions cpp/include/Ice/InputStream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,8 +947,7 @@ class ICE_API InputStream : public IceInternal::Buffer
}
else
{
v = ::IceInternal::createProxy<T>();
v->_copyFrom(proxy);
v = std::make_shared<T>(*proxy);
}
}

Expand Down
94 changes: 43 additions & 51 deletions cpp/include/Ice/Proxy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,6 @@ typedef IceUtil::Handle<ProxyGetConnection> ProxyGetConnectionPtr;
namespace IceInternal
{

template<typename P>
::std::shared_ptr<P> createProxy()
{
return ::std::shared_ptr<P>(new P());
}

inline ::std::pair<const Ice::Byte*, const Ice::Byte*>
makePair(const Ice::ByteSeq& seq)
{
Expand Down Expand Up @@ -278,10 +272,14 @@ class ICE_API ObjectPrx : public ::std::enable_shared_from_this<ObjectPrx>
{
public:

ObjectPrx(const ObjectPrx& other) noexcept;
Copy link
Member Author

Choose a reason for hiding this comment

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

The replacement for _copyFrom .

virtual ~ObjectPrx() = default;

friend ICE_API bool operator<(const ObjectPrx&, const ObjectPrx&);
friend ICE_API bool operator==(const ObjectPrx&, const ObjectPrx&);
/// \cond INTERNAL
ObjectPrx(const IceInternal::ReferencePtr& ref) noexcept : _reference(ref)
{
}
/// \endcond

/**
* Obtains the communicator that created this proxy.
Expand Down Expand Up @@ -1069,8 +1067,6 @@ class ICE_API ObjectPrx : public ::std::enable_shared_from_this<ObjectPrx>

const ::IceInternal::ReferencePtr& _getReference() const { return _reference; }

void _copyFrom(const std::shared_ptr<::Ice::ObjectPrx>&);

int _handleException(const ::Ice::Exception&, const ::IceInternal::RequestHandlerPtr&, ::Ice::OperationMode,
bool, int&);

Expand All @@ -1089,6 +1085,9 @@ class ICE_API ObjectPrx : public ::std::enable_shared_from_this<ObjectPrx>
protected:

/// \cond INTERNAL
// This constructor is never called; it allows Proxy's default constructor to compile.
ObjectPrx() = default;

template<typename R, template<typename> class P = ::std::promise, typename Obj, typename Fn, typename... Args>
auto _makePromiseOutgoing(bool sync, Obj obj, Fn fn, Args&&... args)
-> decltype(std::declval<P<R>>().get_future())
Expand All @@ -1107,22 +1106,19 @@ class ICE_API ObjectPrx : public ::std::enable_shared_from_this<ObjectPrx>
return [outAsync]() { outAsync->cancel(); };
}

virtual ::std::shared_ptr<ObjectPrx> _newInstance() const;
ObjectPrx() = default;
friend ::std::shared_ptr<ObjectPrx> IceInternal::createProxy<ObjectPrx>();
/// \endcond

private:

void setup(const ::IceInternal::ReferencePtr&);
friend class ::IceInternal::ProxyFactory;

::IceInternal::ReferencePtr _reference;
const ::IceInternal::ReferencePtr _reference;
Copy link
Member Author

Choose a reason for hiding this comment

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

ObjectPrx holding this const reference is the pimpl technique:
https://en.cppreference.com/w/cpp/language/pimpl

Note:

  • it's pretty C++ specific so it's odd to use this technique in other languages
  • the "pointer to implementation" is typically a unique_ptr, not a shared_ptr. We should consider switching to unique_ptr when we refactor IceInternal::Reference

::IceInternal::RequestHandlerPtr _requestHandler;
::IceInternal::BatchRequestQueuePtr _batchRequestQueue;
IceUtil::Mutex _mutex;
mutable std::mutex _mutex;
};

ICE_API bool operator<(const ObjectPrx&, const ObjectPrx&);
ICE_API bool operator==(const ObjectPrx&, const ObjectPrx&);

inline bool
operator>(const ObjectPrx& lhs, const ObjectPrx& rhs)
{
Expand Down Expand Up @@ -1163,7 +1159,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_context(const ::Ice::Context& context) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_context(context));
return ::std::make_shared<Prx>(*ObjectPrx::ice_context(context));
Copy link
Member Author

Choose a reason for hiding this comment

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

Before this PR, ObjectPrx::ice_context and friends created a clone of the most derived classes (with virtual newInstance). Now, they just create a new ObjectPrx and we create here the most derived class. It's a bit wasteful but:

a) it doesn't matter since these factory functions aren't called very often
b) when we get rid of shared_ptr for proxies, we can improve all this code

}

/**
Expand All @@ -1173,7 +1169,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_adapterId(const ::std::string& id) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_adapterId(id));
return ::std::make_shared<Prx>(*ObjectPrx::ice_adapterId(id));
}

/**
Expand All @@ -1183,7 +1179,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_endpoints(const ::Ice::EndpointSeq& endpoints) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_endpoints(endpoints));
return ::std::make_shared<Prx>(*ObjectPrx::ice_endpoints(endpoints));
}

/**
Expand All @@ -1193,7 +1189,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_locatorCacheTimeout(int timeout) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_locatorCacheTimeout(timeout));
return ::std::make_shared<Prx>(*ObjectPrx::ice_locatorCacheTimeout(timeout));
}

/**
Expand All @@ -1203,7 +1199,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_connectionCached(bool b) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_connectionCached(b));
return ::std::make_shared<Prx>(*ObjectPrx::ice_connectionCached(b));
}

/**
Expand All @@ -1213,7 +1209,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_endpointSelection(::Ice::EndpointSelectionType type) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_endpointSelection(type));
return ::std::make_shared<Prx>(*ObjectPrx::ice_endpointSelection(type));
}

/**
Expand All @@ -1224,7 +1220,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_secure(bool b) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_secure(b));
return ::std::make_shared<Prx>(*ObjectPrx::ice_secure(b));
}

/**
Expand All @@ -1236,7 +1232,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_preferSecure(bool b) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_preferSecure(b));
return ::std::make_shared<Prx>(*ObjectPrx::ice_preferSecure(b));
}

/**
Expand All @@ -1246,7 +1242,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_router(const ::std::shared_ptr<::Ice::RouterPrx>& router) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_router(router));
return ::std::make_shared<Prx>(*ObjectPrx::ice_router(router));
}

/**
Expand All @@ -1256,7 +1252,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_locator(const ::std::shared_ptr<::Ice::LocatorPrx>& locator) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_locator(locator));
return ::std::make_shared<Prx>(*ObjectPrx::ice_locator(locator));
}

/**
Expand All @@ -1266,7 +1262,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_collocationOptimized(bool b) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_collocationOptimized(b));
return ::std::make_shared<Prx>(*ObjectPrx::ice_collocationOptimized(b));
}

/**
Expand All @@ -1276,7 +1272,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_invocationTimeout(int timeout) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_invocationTimeout(timeout));
return ::std::make_shared<Prx>(*ObjectPrx::ice_invocationTimeout(timeout));
}

/**
Expand All @@ -1285,7 +1281,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_twoway() const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_twoway());
return ::std::make_shared<Prx>(*ObjectPrx::ice_twoway());
}

/**
Expand All @@ -1294,7 +1290,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_oneway() const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_oneway());
return ::std::make_shared<Prx>(*ObjectPrx::ice_oneway());
}

/**
Expand All @@ -1303,7 +1299,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_batchOneway() const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_batchOneway());
return ::std::make_shared<Prx>(*ObjectPrx::ice_batchOneway());
}

/**
Expand All @@ -1312,7 +1308,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_datagram() const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_datagram());
return ::std::make_shared<Prx>(*ObjectPrx::ice_datagram());
}

/**
Expand All @@ -1321,7 +1317,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_batchDatagram() const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_batchDatagram());
return ::std::make_shared<Prx>(*ObjectPrx::ice_batchDatagram());
}

/**
Expand All @@ -1332,7 +1328,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_compress(bool b) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_compress(b));
return ::std::make_shared<Prx>(*ObjectPrx::ice_compress(b));
}

/**
Expand All @@ -1343,7 +1339,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_timeout(int timeout) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_timeout(timeout));
return ::std::make_shared<Prx>(*ObjectPrx::ice_timeout(timeout));
}

/**
Expand All @@ -1354,7 +1350,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_connectionId(const ::std::string& id) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_connectionId(id));
return ::std::make_shared<Prx>(*ObjectPrx::ice_connectionId(id));
}

/**
Expand All @@ -1365,7 +1361,7 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_fixed(const ::std::shared_ptr<::Ice::Connection>& connection) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_fixed(connection));
return ::std::make_shared<Prx>(*ObjectPrx::ice_fixed(connection));
}

/**
Expand All @@ -1376,14 +1372,14 @@ class Proxy : public virtual Bases...
*/
::std::shared_ptr<Prx> ice_encodingVersion(const ::Ice::EncodingVersion& version) const
{
return ::std::dynamic_pointer_cast<Prx>(ObjectPrx::ice_encodingVersion(version));
return ::std::make_shared<Prx>(*ObjectPrx::ice_encodingVersion(version));
}

protected:

/// \cond INTERNAL
virtual ::std::shared_ptr<ObjectPrx> _newInstance() const = 0;
/// \endcond
// This constructor never initializes the base classes since they are all virtual and Proxy is never the most
// derived class.
Proxy() = default;
};

ICE_API ::std::ostream& operator<<(::std::ostream&, const ::Ice::ObjectPrx&);
Expand Down Expand Up @@ -1494,8 +1490,7 @@ uncheckedCast(const ::std::shared_ptr<T>& b)
r = ::std::dynamic_pointer_cast<P>(b);
if(!r)
{
r = IceInternal::createProxy<P>();
r->_copyFrom(b);
r = std::make_shared<P>(*b);
}
}
return r;
Expand All @@ -1516,8 +1511,7 @@ uncheckedCast(const ::std::shared_ptr<T>& b, const std::string& f)
::std::shared_ptr<P> r;
if(b)
{
r = IceInternal::createProxy<P>();
r->_copyFrom(b->ice_facet(f));
r = std::make_shared<P>(*(b->ice_facet(f)));
}
return r;
}
Expand All @@ -1540,8 +1534,7 @@ checkedCast(const ::std::shared_ptr<T>& b, const ::Ice::Context& context = Ice::
{
if(b->ice_isA(P::ice_staticId(), context))
{
r = IceInternal::createProxy<P>();
r->_copyFrom(b);
r = std::make_shared<P>(*b);
}
}
return r;
Expand Down Expand Up @@ -1569,8 +1562,7 @@ checkedCast(const ::std::shared_ptr<T>& b, const std::string& f, const ::Ice::Co
::std::shared_ptr<::Ice::ObjectPrx> bb = b->ice_facet(f);
if(bb->ice_isA(P::ice_staticId(), context))
{
r = IceInternal::createProxy<P>();
r->_copyFrom(bb);
r = std::make_shared<P>(*bb);
}
}
catch(const Ice::FacetNotExistException&)
Expand Down
Loading
Loading