On 03/18/13 17:35, Adriano dos Santos Fernandes wrote:
> On 18/03/2013 07:46, Alex Peshkoff wrote:
>> On 03/18/13 03:40, Adriano dos Santos Fernandes wrote:
>>
>>> - IStatus (1):
>>>
>>> It's not clear what is this when interacting with external code.
>>> Provider functions clears it when entering but no other function does
>>> it. It's not clear what callers should do to check states.
>> All called functions should clear it - that's how ISC API always used to
>> work with passed status vector. If someone does not - that's a bug.
> This is not the fastest way! To called initialize status, it must call
> virtual function init every time.
>
> On the other hand, if caller need to init, it could be inline. In this
> case, no virtual function call is necessary to check errors. Say:
>
> MyStatusImpl status; // inline
> apiFunction(&status); // status methods are called only if a error is
> throw
> status.check() // inline
>
> So I have doubts about the provider manner of calling init.
Old, well-known conflict between performance and flexibility :)
But OK, let's go in this direction if nobody objects. In fact 99, 9999%
of that inits will be useless.
What's not good is that we add one more 'hidden' difference with old api
and in an aspect where it was rather clear.
And we have some troubles when we plan to analyze returned IStatus in
cases when we pass already existing interface to somebody else. Not
typical case, but if we say that in case of no-error status is not
changed we will have to add own status variable and probably copy it
back to passed to us interface. Telling that on no-error state of
IStatus is implementation-defined does not look like good idea.
>
>>> - IStatus (2):
>>>
>>> It should manage its memory. Depend on circular buffers in its
>>> implementation is just wrong.
>> Certainly. But this is not IStatus itself problem, it's implementation
>> problem. Memory should be managed by IStatus, but this should never be
>> expressed in any way in interface itself, moreover in some hidden way.
>> Remember that user may implement IStatus himself.
> But then engine internal should just not care. Having a circular buffer
> for user's IStatus makes them (and our own code) not care on requirements.
>
> Only legacy y-valve functions should rely on the circular buffer IMO.
Once again - I agree with what you suggest. Moreover - avoiding circular
buffer was in initial plan when adding IStatus, but implementation is
not ready, and we talked about it privately. Here I just wanted to pay
attention to some details.
BTW, to make it all be used efficiently in engine afraid we need more
(much more) changes than for adding strings memory management to
IStatus. Therefore great desire to solve first what to do with warnings
(how to store them in IStatus better), next change engine code (where
that warnings arrive).
>>> - Automatic Exception->IStatus and IStatus->Exception conversions:
>>>
>>> I tried to implement this, macros would generate methods that check
>>> status automatically or catch-and-convert exceptions. In the end, I just
>>> lost time. Mentioned IStatus problems and usage of warnings as you want
>>> to maintain make that an impossible task.
>>>
>>> Current code that manually enclose or check exceptions are ugly and
>>> error-prone.
>> I do not like use of macros in the code cause they prevent debugging.
>> But may be in this case they may be used with care....
> Yes, but have status.check() and try { ... } catch { ... stuff ... }
> everywhere is neither elegant.
I talk first of all not about elegance, but about having debuggable
code. If (for example) boost becomes standard de facto and therefore
debiggers are learned to take it into an account, I will certainly agree
with using macros in such cases.
>> What about warnings - it's more interesting thing. According to standard
>> calls to database engine may raise *exception* and *completion*
>> conditions with completion conditions divided into *no data*, *warnings*
>> and other completion conditions. Sooner of all we should reflect that in
>> IStatus, but currently I have no good ideas how.
>>
>>> - IMaster:
>>>
>>> In a perfect world, we would not have static variables, but variables in
>>> singletons (per-engine/yvalve). In this perfect world, an user would be
>>> able to load two different yvalve (embedded client) and a single engine
>>> DLL would work if loaded in the two different yvalve. I don't want to
>>> propose this requirement, but IMO, that usage of how code (engine,
>>> plugins, etc) catches the IMaster pointer is just ugly.
>>>
>>> AFAIK, it will make impossible that user loads two yvalve with then
>>> loads two different engines
>> Sorry, but some software (like IBE) makes it possible to explicitly
>> specify client library to use with specific database. I.E. unfortunately
>> we must be ready to have >1 yvalve per process.
> And this is what I saw it's required to support, but do you think the
> current code allows it?
As far as I know - yes, it does.
>>> , as the engines will call a yvalve function
>>> (fb_get_master) who cannot be guessed from what library it must be
>>> called. IMO, IMaster should being passed as parameters, and not like
>>> current.
>> PluginEntrypoint* startModule;
>> if (module->findSymbol(STRINGIZE(FB_PLUGIN_ENTRY_POINT),
>> startModule))
>> {
>> startModule(masterInterface);
>>
>> Removed some code to make it visible better that IMaster is passed here
>> as a parameter to plugin module at startup. If you prefer another form
>> ("not like current"), please suggest.
> Usage of CachedMasterInterface in jrd.cpp seems correct.
>
> But what about this code?
>
> IMaster* CachedMasterInterface::getMasterInterface()
> {
> if (!cached)
> {
> cached = fb_get_master_interface();
> }
> return cached;
> }
>
> If you have 3.0 and 3.1 engines loaded in memory, each one correctly
> loaded by different client/y-valve libraries, how do you think when this
> code is called inside the engine, it will get the correct IMaster?
Yes. It will get IMaster from yvalve which loaded an engine.
>
> Many places calls MasterInterfacePtr, PluginManagerInterfacePtr and
> others that will not work correct.
The question is what is called correct. In plugins there will always be
present cached IMaster. In external code (currently it's only remote
server if I'm not missing something) fb_get_master_interface() from the
library loaded at startup time top resolve external references will be
used. On my mind that's correct.
If some plugins do not get access to particular engine due to badly
configured system - hmm, sooner of all people should not load multiple
clients if (for example) they want to have in external routines
cross-database calls to databases with other ODS.
>>> - Checkouts/Checkins:
>>>
>>> The interaction of checkouts and checkins in the engine is not clear in
>>> regard to plugins. AFAIK this was not necessary (external engines first
>>> implementation work without some of them introduced later), cause the
>>> reentrant locks still work without them. If it's not necessary, why do
>>> some of them (checkouts) were introduced?
>> Can you be more specific here? What checkouts do you not like?
> It's not about I like/dislike. For example, before I implemented
> IMetadataBuilder, your recommendation was that user would need to
> implement IMessageMetadata getter methods, and with the current API this
> is still possible. But you do not implement IMessageMetadata getter
> methods with checkouts. How user must know if he can enter in the engine
> from this methods or not? If he does, it will happen a checkin without a
> checkout.
>
> This is what I mean. And if it works, why the checkout is necessary on
> the more obvious paths?
Now I understand a problem. Well, we even do not need new
IMessageMetadata - suppose it's possible to find a place where IStatus
methods are called w/o checkout. And engine's behavior will be
definitely not good. I think that for interfaces like mentioned
IMessageMetadata (moreover - IStatus) it's better to prohibit API calls.
With check certainly. Not good that this check itself requires
resources. May be better ideas? Checkout when we are going to use such
methods? For IStatus for example it's almost not needed - it's set()
must be called only when receiving exceptions from inner layers. Calls
to IMessageMetadata() methods are also well grouped together.
>>> What if a checked-out plugin
>>> codes calls a non-provider API function that enters deep in the engine?
>>> This problems is also related with the tdbb still being used.
>> Do we still have non-provider API functions that may enter deep in the
>> engine? This definitely worth solving.
> The important word here is "may", as explained above. It's plugin writer
> who decide things. We must then define what's is possible, impossible
> (error conditions) or undefined behaviours.
Forget it. I just did not understand what means function that enters
deep in the engine here.
>
>>> - Pools x refcounters:
>>>
>>> I personally hate to use pools (and yes, I hate the refcounters too) and
>>> the cost of carrying them, but at least it was consistent in usage. But
>>> now, refcounted objects are created in the global pool, while it's
>>> internal objects may or may not be in the same pool.
>> Yes. And that's not a problem, though seems to be so at the first
>> glance. With refcounted object in global pool we provide stable pointer
>> to the outside world (and internal async calls: logically they do not
>> differ much from external API calls), that mini-object checks presence
>> of BIG internal object (which may be created in another pool), and if
>> present - calls it. Nothing bad happens.
> Another thing I want to be solved is code like that:
>
> RefPtr x(functionThatReturnRefWith1Count());
> if (x)
> x.release();
> // use x
>
> This is very ugly. But maybe a simple second parameter in RefPtr may
> solve it.
Yes, worth thinking and doing.
>
>>> We cannot use the std C++ library. We rewrote thread
>>> support,
>> Hmm...
>> Something new for me. There was a win-only code which solved
>> response-time problems with old thread manager. They were pre-fb3, but
>> both are gone in fb3.
>> Is it degradation?
>>
>>> we rewrote multi-thread sync. objects and many things available.
>> As long as existing standard objects satisfy our requirements we use
>> them. But what to do if we need something more?
> But sometimes looks that Firebird is the only software who push the
> limits. I think in many cases existing boost/std libraries may solve our
> problems like they solves everyone else problems.
Don't know. Probably.
If we already have working solution I do not volunteer to look for
presence of analogue...
One think may be said for sure - I do not remember cases when we added
own objects instead of working for us standard one.
>>> We cannot write C++ code usable internally in Firebird and available to
>>> our C++ users.
>> And what about Delphi users who need same fucntionality? [Almost sure
>> that number of them > number of C++ users. ]
> But Firebird is coded in C++, and my point was to externalize our things
> that makes usage of API (in C++) easy. But if this code depends on
> entire Firebird common library and does not uses standard C++
> facilities, no body would want to integrate our code in their projects.
API should not be oriented on how internals are coded, including what
language is used for it.
>>> We have our strings, we have our exceptions and everyone
>>> should adapt to it.
>> That wonderful std::string missing even trivial trim() function? No, please!
>> Well, I do not remember details why we stopped to use std::exception.
>> Something like it was suggested by Jim many years ago.
Remembered main problem. For correct use of std::exception RTTI was
needed, which seemd to be a problem for some OS (Mac?).
With all this new++ technologies the main question comes - what does
project win with such code rework? Except possibility of problems on
some platforms :( May be I miss some obvious pluses...
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_mar
Firebird-Devel mailing list, web interface at
https://lists.sourceforge.net/lists/listinfo/firebird-devel
