les: implement new client pool #19745

zsfelfoldi · 2019-06-20T14:43:17Z

This PR implements a new client pool which replaces (and is based on) the old freeClientPool.
Important features:

It puts free and prioritized clients in the same pool using a shared priority model
Introduces the concept of positive and negative balance. Positive balance is externally assigned and translates to higher connection priority while negative balance (which translates to lower priority) is a virtual cost calculated for free service and it is being exponentially reduced when the client is not connected (just like recentUsage in the old freeClientPool).
implements prque.LazyQueue which is a smart priority queue capable of handling continously changing priorities and recalculating them on demand. This also allows calculating not just time cost but request cost too for free clients (which the old pool did not do) which will hopefully improve service availability by penalizing the heavy requesters

Note: this PR is based on top of #19543 so only the second commit is relevant.

Note2: this PR removes the half-finished priority handling API (but leaves the checkpoint functions of course). The API will be polished a bit more and added in a subsequent PR, probabliy in a subsequent release. The back end for most of the interesting functionality is in the new pool but it would be nice to test its basic functionality a bit before adding new and complex possible use cases.

common/prque/lazyqueue.go

les/balance.go

les/clientpool.go

common/prque/lazyqueue.go

zsfelfoldi · 2019-06-26T19:26:19Z

@karalabe @rjl493456442 I removed the csv logger and added metrics instead

les/clientpool.go

common/mclock/simclock.go

rjl493456442 · 2019-07-22T04:34:50Z

les/clientpool.go

+func (f *clientPool) finalizeBalance(c *clientInfo, now mclock.AbsTime) {
+	c.balanceTracker.stop(now)
+	pos, neg := c.balanceTracker.getBalance(now)
+	pb := f.getPosBalance(c.id)


Can we also store the posBalance into database if a client is dropped. Currently we will maintain the positive balance in Queue(which is cache) and flush items if the size of queue exceeds the limit(of course we will flush all positive balances when clientpool is closed). So if the Geth node is crashed, we can lose a lot of positive balances

We save them immediately when the balance is increased but I think we can also save at disconnection. Will add it soon.

rjl493456442 · 2019-07-22T04:49:45Z

les/server.go

-
+	s.fcManager.SetCapacityLimits(s.freeClientCap, s.maxCapacity, s.freeClientCap*2)
+	s.clientPool = newClientPool(s.chainDb, s.freeClientCap, 10000, mclock.System{}, func(id enode.ID) { go s.protocolManager.removePeer(peerIdToString(id)) })
+	s.clientPool.setPriceFactors(priceFactors{0, 1, 1}, priceFactors{0, 1, 1})


I think the time cost is a bit too high.

E.g. for a paid client(cap=1000,000). If the client connects 1s, the time cost is 10^9. While for a normal request cost is much lower.

Besides, we will also consider time cost for estimated balance. The refresh interval in lazyQueue is 10s.
It means the estimated balance will much much higher than the actual balance. It can result in we always need to dump all items from estimated queue and then push them to pop queue to pick item.

Currently I think there is probably no "theoretically optimal" value for the ratio of time and request costs and the current default value is just a wild guess but I think it is in a reasonable range. As you wrote, 1s of connection with 1M capcacity costs 10^9 units. 1M capacity means that 10^9 request cost per second is guaranteed so if you use your guaranteed amount then the request cost will be equal to the time cost. If only a few clients are requesting then they also get extra buffer recharge which allows for up 10 times higher request rate so if the client is sucking data at maximum rate then the request cost can be even 10 times higher than the time cost.

Okay. Can we add a metric here to collect the average number of items be popped for MultiPop function? I have a bad feeling that we will pop the whole queue.

rjl493456442 · 2019-07-22T04:58:28Z

I am thinking whether it's reasonable to evict a paid client(positive balance > 0) from the clientpool.
If positive balance >0, it means the client pays for the service and the money is not used up now. It's not user-friendly to kick out a paid client.

zsfelfoldi · 2019-07-22T12:54:21Z

@rjl493456442 positive balances can also be very small. I don't think we should treat someone with 1 wei positive balance drastically differently from someone with 0. Kicking someone out (even a paid client) is always acceptable because you can always turn off your server or technical problems can happen. It is always up to the client to decide whether the service was worth the price and whether it wants to pay again. If you must upset one of your paying clients a little bit because you have limited capacity then at least be nice to the best clients.
Also, in my model the server can always put a price tag on letting a certain client in. The client will be able to ping the server to ask its status (maybe a cheap UDP exchange is enough) and the server can tell "pay me this much before attempting to connect". This can save a lot of failed connection attempts and also make it easier for clients to find the currently least overloaded server (a bit similar to the reasoning behind the flow control). This client pool priority design makes it easy to determine the amount to be paid for anyone to connect and this value is going to be a good signal if there is too much competition to connect to a certain server.

rjl493456442 · 2019-07-22T14:39:18Z

@zsfelfoldi Thanks for the reply.
Regarding the positive balance, yes uniform the logic for paid client and free client can clean the code a bit. So let's do this. Even when a paid client is kicked out, I think the positive balance information can also be kept. So next time when the client connects again, we can still assign it a "high" priority.

rjl493456442 · 2019-07-22T14:48:05Z

I am running two Geth instances in my local with this PR. If everything works well, I think we can merge this PR in 1.9.2 :))

rjl493456442

Please rebase and fix these issues. And also please save the positive balance when we drop the client. Otherwise, PR looks good to me.

common/prque/lazyqueue.go

rjl493456442 · 2019-07-23T02:23:03Z

les/clientpool.go

+	}
+	c.priority = false
+	if c.capacity != f.freeClientCap {
+		f.connectedCapacity += f.freeClientCap - c.capacity


Does it work?

I have a test snippet

a := uint64(64) a += uint64(10) - uint64(32) fmt.Println(a)

And the output is constant -22 overflows uint64

Sure, on the machine code level there is absolutely no difference between signed and unsigned addition/subtraction. If you specify constants then the compliler will check for overflows but this is not the case here. I could typecast it to int64 but typecasting is also an implicit assumption about value range and I do not think it would make the code look better. If it an established standard though (or at least done everywhere else in our codebase) then I can add a typecast in these cases. In that case I have to check the rest of my code too though because I think I do this regularly :)

One other example is prque.Prque which uses a "wrap-around" priority scheme where the comparison is int64(a-b) in order to allow perpetually moving offsets. Binary integers are modulo values technically and I have always considered it perfectly "legal" to think about them and use them that way.

rjl493456442 · 2019-07-23T02:30:05Z

les/clientpool.go

+	}
+	if setTotal {
+		if pb.value+amount > pb.lastTotal {
+			pb.value += amount - pb.lastTotal


Also, a test case here

pb.value, amount, pb.lastTotal := uint64(5), uint64(10), uint64(14) amount - pb.lastTotal < 0, constant -4 overflows uint64

rjl493456442

LGTM

zsfelfoldi added this to the 1.9.0 milestone Jun 20, 2019

zsfelfoldi requested review from fjl, gballet, holiman, karalabe and rjl493456442 as code owners June 20, 2019 14:43

zsfelfoldi added the pr:review label Jun 20, 2019

zsfelfoldi force-pushed the new-clientpool branch from 7ecf6d4 to 7b61049 Compare June 20, 2019 21:56

rjl493456442 self-assigned this Jun 21, 2019

rjl493456442 requested changes Jun 26, 2019

View reviewed changes

rjl493456442 reviewed Jun 26, 2019

View reviewed changes

les/clientpool.go Show resolved Hide resolved

common/prque/lazyqueue.go Show resolved Hide resolved

rjl493456442 reviewed Jun 27, 2019

View reviewed changes

les/clientpool.go Show resolved Hide resolved

les/clientpool.go Show resolved Hide resolved

les/clientpool.go Outdated Show resolved Hide resolved

adamschmideg modified the milestones: 1.9.0, 1.9.1 Jun 27, 2019

rjl493456442 force-pushed the new-clientpool branch from 88113e0 to 57449c2 Compare July 1, 2019 05:03

rjl493456442 reviewed Jul 1, 2019

View reviewed changes

common/mclock/simclock.go Show resolved Hide resolved

zsfelfoldi force-pushed the new-clientpool branch from 57449c2 to db03558 Compare July 20, 2019 23:19

rjl493456442 reviewed Jul 22, 2019

View reviewed changes

rjl493456442 reviewed Jul 23, 2019

View reviewed changes

karalabe modified the milestones: 1.9.1, 1.9.2 Jul 23, 2019

zsfelfoldi force-pushed the new-clientpool branch from db03558 to ec489e6 Compare July 24, 2019 12:07

rjl493456442 approved these changes Jul 24, 2019

View reviewed changes

zsfelfoldi force-pushed the new-clientpool branch 2 times, most recently from 936e30d to b2aa249 Compare August 2, 2019 14:56

zsfelfoldi force-pushed the new-clientpool branch from b2aa249 to 58865cb Compare August 2, 2019 15:25

les: implement new client pool

cd4eb2a

zsfelfoldi force-pushed the new-clientpool branch from 58865cb to cd4eb2a Compare August 2, 2019 17:47

zsfelfoldi merged commit a7de796 into ethereum:master Aug 3, 2019

les: implement new client pool #19745

les: implement new client pool #19745

Uh oh!

Conversation

zsfelfoldi commented Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zsfelfoldi commented Jun 26, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjl493456442 commented Jul 22, 2019

Uh oh!

zsfelfoldi commented Jul 22, 2019

Uh oh!

rjl493456442 commented Jul 22, 2019

Uh oh!

rjl493456442 commented Jul 22, 2019

Uh oh!

rjl493456442 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjl493456442 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zsfelfoldi commented Jun 20, 2019 •

edited

Loading