Bugzilla Version 2.16.7
Bugzilla Bug 1892
  allow to forget read only objects in long batch processes
     Query page      Enter new bug
Bug#: 1892   Platform:   Reporter: martin-fuchs at gmx dot net (Martin Fuchs)
Product:   OS:   Add CC:
Component:   Version:   CC:
Remove selected CCs
Status: ASSIGNED   Priority:  
Resolution:   Severity:  
Assigned To: martin-fuchs at gmx dot net (Martin Fuchs)   Target Milestone:  
URL:
Summary:

Attachment Type Modified Status Actions
proposed patch patch 2005-02-27 09:00 none Edit
Patch to make usage of AccessMode public patch 2005-03-26 14:32 none Edit
Create a New Attachment (proposed patch, testcase, etc.) View All

Bug 1892 depends on: 1933 Show dependency tree
Show dependency graph
Bug 1892 blocks:
Votes: 0    Show votes for this bug    Vote for this bug

Additional Comments:


Leave as ASSIGNED 
Resolve bug, changing resolution to
Resolve bug, mark it as duplicate of bug #
Reassign bug to
Reassign bug to owner of selected component

View Bug Activity   |   Format For Printing

Description: Opened: 2005-02-27 08:59

When using Castor to process data in a long read only batch process the fetched 
objects are all stored in TransactionContext._readOnlyObjects. This hash table 
is not cleared until the end of the transaction. So there may arise problems 
running out of heap memory.
Well, one way to overcome this would be to use shorter transactions. But I 
think it's better to provide a simple additional method to disable usage of the 
read-only-objects hash table. It avoids reconnecting to the database very 
frequently and increases the performance of the batch process.
Attached patch implements the new function Database.setRememberReadOnlyObjects
() to deactivate the read only hash.

------- Additional Comment #1 From Martin Fuchs 2005-02-27 09:00 -------
Created an attachment (id=943)
proposed patch

------- Additional Comment #2 From Martin Fuchs 2005-03-05 05:57 -------
Well, what does everybody think about this extension?
We already used this successfully in our application.

------- Additional Comment #3 From Martin Fuchs 2005-03-06 02:59 -------
*** Bug 1450 has been marked as a duplicate of this bug. ***

------- Additional Comment #4 From Werner Guttmann 2005-03-08 07:38 -------
Martin, I think your patch is quite clear and precise. I wonder, though, 
whether we should actually overload Databaseload() to provide this 
functionality, and not add a property to Database(Impl) ?

------- Additional Comment #5 From Werner Guttmann 2005-03-15 07:02 -------
Just a question: I think that somewhere in the PAI, Castor JDO allows you to
elevate a lock from 'read' to something higher, e.g. 'write'. How would this
interfer with your ideas ? 

------- Additional Comment #6 From Werner Guttmann 2005-03-16 00:47 -------
Btw, if you wondered about what PAI stands for, don't worry .. ;-). I meant to
say 'API', of course.

------- Additional Comment #7 From Gregory Block 2005-03-16 01:35 -------
I'm of two minds on this.

Either:  Read only objects are critical to keep, in which case the hashtable is a correct implementation 
and a flag either on load or on the database object is correct behavior...

Or: Keeping these things around forever is pointless, and moving to a weak reference is the right 
solution; if the application didn't want it, and we're not referencing it, it can go away.

The weak hash ref is probably the easiest transparent change to make, and will *likely* behave correctly 
in all normal circumstances; it also provides a mechanism for knowing when something used to be 
loaded but is no longer present, as the key is still in the weak hash reference, and debug output can 
inform the user that memory pressure led to the application clearing that content.

A flag, instead of setting whether or not read only objects should be remembered, could then allow you 
to select reference type:  Soft (eliminate when the JVM catches an OOM exception and wants to purge 
unnecessary objects from the system), weak (eliminate whenever a GC pass detects an unreferenced 
object),  phantom (eliminate when all other references are gone, if the application uses weak 
references itself).

Implementing a switch that allows you to select the referent style - strong, weak, soft, or phantom - 
then provides for today's behavior (a 'strong' reference, which just holds an ordinary link to the object 
in the value), and provide for three situations:  Discard unreferenced on GC (soft), discard unreferenced 
under memory pressure (weak), and discard unused on GC (phantom).

Drawback:  it requires you to create reference objects, which increases the number of objects we have 
to handle.

------- Additional Comment #8 From Werner Guttmann 2005-03-16 02:00 -------
What's a good article on weak/phantom references ? It looks like I need to
educate myself a bit .. :-(.

------- Additional Comment #9 From ralf joachim 2005-03-16 02:21 -------
The requested feature only relates to access mode ReadOnly and can not be used
with other access modes. As suggested by Martin someone that wants to use the
new feature would have to call setRememberReadOnlyObjects() and call load() or
execute() with access mode ReadOnly. A drawback of the suggested solution is
that it will not be possible to load objects with access mode ReadOnly that are
remembert in the transaction with others that should not been remembert.

I suggest to introduce a new access mode ReadOffTransaction to implement the
requested feature. This would not have the disadvantages from above.

In addition we need to think about what will happen if the requested object is
already part of the transaction. I think it have to remain in the transaction
and the load() or execute() with access mode ReadOffTransaction behaves as with
access mode ReadOnly for this object only.

I don't think we can use soft-, weak- or phantom-references in
TransactionContext because we have to take care to commit or rollback all
changes done during the transaction and also all locks have to be released at
the end of the transaction.

------- Additional Comment #10 From ralf joachim 2005-03-16 02:25 -------
The javadoc is not to bad but I can also search for some articles in magazines
if you like.

------- Additional Comment #11 From Werner Guttmann 2005-03-16 02:28 -------
Don't worry, Ralf. Unless a particular article springs to your mind immediately,
I'll be happy to browse myself.

------- Additional Comment #12 From Werner Guttmann 2005-03-16 02:33 -------
> In addition we need to think about what will happen if the requested 
> object is already part of the transaction. I think it have to remain 
> in the transaction and the load() or execute() with access mode 
> ReadOffTransaction behaves as with access mode ReadOnly for this object 
> only.
Without commenting upon the feasability of this suggested approach, this looks 
to me like we need to add even more if-else-ifs to an already quite convoluted 
code base.

------- Additional Comment #13 From Gregory Block 2005-03-16 10:07 -------
Java in a Nutshell is my bible on references behavior.

As for long-term stuff;  Basically, it's read-only.  I'm only talking about modifying the read-only portion 
of the transaction, and it's under the assumption that anything in the read-only section can safely go 
away.  If that's an invalid assumption, say so.  :)

However, it's *probably* safe to use references for anything in the read-only section.

------- Additional Comment #14 From Werner Guttmann 2005-03-16 14:23 -------
> ... and it's under the assumption that anything in the read-only section 
> can safely go away.
As mentioned above, a read lock for an object can be upgraded. If we 'did away'
with that object, this could be a problmem.

------- Additional Comment #15 From Martin Fuchs 2005-03-20 06:10 -------
> I suggest to introduce a new access mode ReadOffTransaction to implement the
> requested feature. This would not have the disadvantages from above.

Would be fine with me. But could you please explain what the 
name "ReadOffTransaction" stands for? I could think about a new constant with a 
name like "PlainRead" or "BatchRead" in the Database interface.

> In addition we need to think about what will happen if the requested object is
> already part of the transaction. I think it have to remain in the transaction
> and the load() or execute() with access mode ReadOffTransaction behaves as 
with
> access mode ReadOnly for this object only.

Perhaps someone can explain me why the accessMode is translated into an 
AccessMode object in DatabaseImpl.load() ?
Dropping this mapping and just using the enumeration values could perhaps make 
the code a bit more readable.

------- Additional Comment #16 From ralf joachim 2005-03-20 07:08 -------
>Would be fine with me. But could you please explain what the 
>name "ReadOffTransaction" stands for? I could think about a new constant with a 
>name like "PlainRead" or "BatchRead" in the Database interface.

I used 'ReadOffTransaction' as it is a read only without adding the objects read
to the transaction but I also didn't like that name. Maybe your ideas are
better. Let's hear suggestions from our contibutors speaking native english ;-)

>Perhaps someone can explain me why the accessMode is translated into an 
>AccessMode object in DatabaseImpl.load() ?
>Dropping this mapping and just using the enumeration values could perhaps make 
>the code a bit more readable.
I guess that this has historical reasons and that the one that introduced
AccessMode don't wanted to change Database interface. If we want to do that now
we first need to introduce new db.load(... AccessMode) methods and declare the
old ones as deprecated until we remove them some versions later. We also can
make code a little easier to read by delegating the short->AccessMode
translation to the AccessMode class.

------- Additional Comment #17 From Martin Fuchs 2005-03-24 01:36 -------
Well, I though of keeping the Database interface as it's now (just adding a new 
constant for the read-only-dont-remember mode) and dropping the AccessMode 
class.

------- Additional Comment #18 From ralf joachim 2005-03-24 06:44 -------
If you search where the AccessMode class is used you may recognize that it will
be rather complicated to remove it (211 references). In addition I prefer
enumerations like AccessMode over unrelated short primitives.

I'll prepare a patch about my ideas to modify AccessMode and clean up Database
and DatabaseImpl.

------- Additional Comment #19 From ralf joachim 2005-03-26 14:32 -------
Created an attachment (id=989)
Patch to make usage of AccessMode public

------- Additional Comment #20 From ralf joachim 2005-04-12 15:22 -------
Moved refactoring of AccessMode to bug 1933

     Query page      Enter new bug
This is Bugzilla: the Mozilla bug system. For more information about what Bugzilla is and what it can do, see bugzilla.org.
Actions: New | Query | bug # | Reports   New Account | Log In