4

I have a conceptually simple application: data comes in as small encrypted packets, they decrypted and validated, some are stored in the database, some rules are applied, and a reply might be encrypted and sent. Packets arrive at a rate of high hundreds of thousands per thread per second, so performance is a consideration. I am leaving out most of the performance elements here to focus on the design.

Main elements: packets arrive in a queue and each processor thread gets one at a time:

  1. take ownership of a single incoming packet (at a time)
  2. decrypt
  3. use a factory to construct the appropriate packet class
  4. packet saves itself to database (often multiple inserts)
  5. rule engine run by some types of packets, resulting actions queued
  6. packet may generate 1 or more replies to 1 or more recipients

One problem is that each processor thread owns a bunch of helper instances that get passed into the constructor of the packets. Those helper classes are often painful to create (like the database connection), so I don't want the packet to construct them. During decryption I accumulate information about the packet from the database so I want to pass that in too. That's a lot of parameters.

I've ended up with 15 parameters to the PacketClass constructor and growing. That seems excessive, but I can't see an elegant solution.

The constructor looks like this:

PacketClass(database, cryptoEngine, {more references} ...
   packetDataBlob, packetBlobSize, {packet source information}, 
   {database values passed rather than repeating lookups}...

At some stage I intend to cache the database info that gets looked up during decryption because there's only going to be a million or so records and that lookup will be a bottleneck. But for now there's ~50 bytes in 6 values that get looked up for each packet then passed into the constructor from one SELECT query. Once the cache is operational those will become a single const reference but for now they're not.

Options:

  • make all the utility class references global or thread-global as per the IOC/service locator trend
  • pass a reference to the owning thread/processor to each packet giving quite horrid linking
  • wrap the params into a couple of data classes and pretend there's nothing to see.
  • split the PacketClass constructor into construct-with-references and initialise-with-data methods (does this even count as a solution?)
  • some brilliant solution I haven't thought of

Can anyone suggest options?


Robert Harvey suggested in chat splitting the processing into a PacketProcessorClass for each PacketClass so that behaviour and data are separated. That way much of the processing is done to the PacketClass in the processor thread rather than inside the packet as it is now. I'm going to work through that design over the weekend. Other ideas are also welcome.

New Design:

PacketClass( packetDataBlob, packetBlobSize, {packet source information}, 
   {database values passed rather than repeating lookups}...

PacketClassProcessor(database, cryptoEngine, {more references})
PacketClassProcessor.Process(Packet)

Performance considerations: this might even be faster than the current setup. I expect to be able to cache most of the ProcessorClass instances, so there's not that construction overhead. And the PacketClass could become a C struct so again no constructor overhead except for any collections it contains... and I can cache at least one of those (the "rules that apply to this packet' one).

(I edited out some fluff too)

3
  • Let us continue this discussion in chat. Commented Nov 21, 2014 at 3:43
  • 1
    To bring closure to this question, can the suggested answer from chat be posted as the answer? Assuming it worked out that is.
    – J Trana
    Commented Jan 14, 2015 at 3:21
  • Sorry for the late, late comment: I have ended up dumping the whole "pretend it's OO" sham and now each processor thread owns the helper classes and has a case statement and a set of "process one type of packet" methods. Creating each thread has a pile of genuinely global references (config class etc) and a set of fields that were once passed around as parameters. It's simpler and faster, but it is a 3000 line class. Introducing the lookup caches made it clear that the PacketFactory was doomed (although I did use a pair of Parameter Objects while discovering that)
    – Móż
    Commented Jun 21, 2015 at 11:28

1 Answer 1

2

It sounds like you need the classic refactoring Introduce Parameter Object, which might be where Robert Harvey is heading with PacketClassProcessor. Basically, if a large clump of variables are passed around between multiple methods, wrap all (or some) of them in a parameter class.

1
  • 1
    I think it's more than parameter objects. PacketClassProcessor takes over a large portion of responsibility away from the current design of PacketClass (which shouldn't belong to the latter anyway), and PacketClassProcessor is also becoming stateful, because the statistics of previously received packets will influence the actions performed by PacketClassProcessor for new packets. This is a far more deeper refactoring than parameter objects.
    – rwong
    Commented Apr 26, 2015 at 16:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.