Hello,
Problem
Converting a date to lunar and back walks the year table instead of indexing it.
FromStdTime starts at minYear and steps towards the wanted year, calling
getDaysInYear on every step (calendar/lunar/lunar.go:90):
for l.year = minYear; l.year <= maxYear && offset > 0; l.year++ {
daysInYear = getDaysInYear(l.year)
offset -= daysInYear
}
and getDaysInYear recounts the month bits of that year every time:
func getDaysInYear(year int) int {
var days = 348
for i := 0x8000; i > 0x8; i >>= 1 {
if (years[year-minYear] & i) != 0 {
days++
}
}
return days + getDaysInLeapMonth(year)
}
ToGregorian has the same shape through getOffsetInMonth, which sums the
length of every year since minYear:
func getOffsetInMonth(year int) int {
offset := 0
for y := minYear; y < year; y++ {
offset += getDaysInYear(y)
}
return offset
}
So both conversions cost O(years since 1900), and the cost is visible in the
benchmarks:
| date |
to lunar |
back to gregorian |
| 1901-02-05 |
107 ns |
31 ns |
| 2020-08-05 |
1009 ns |
738 ns |
| 2099-08-05 |
1504 ns |
1221 ns |
A CPU profile of the conversion spends 26.6% of its time in getDaysInYear.
Suggested change
Everything those loops compute follows from the static years table, so it can
be derived once: the number of days in each supported year, and the running
total of those days since minYear. getDaysInYear then becomes a lookup and
getOffsetInMonth becomes O(1). The two tables are 201 ints each, built at
package initialisation.
No logic changes, only the two accessors:
| date |
to lunar |
back to gregorian |
| 1901-02-05 |
107 ns -> 97 ns |
31 ns -> 30 ns |
| 2020-08-05 |
1009 ns -> 282 ns (-72%) |
738 ns -> 23 ns (-97%) |
| 2099-08-05 |
1504 ns -> 362 ns (-76%) |
1221 ns -> 22 ns (-98%) |
Correctness
Because both accessors are memoized pure functions of the same table, the
output cannot change, and I checked it rather than assuming: for every one of
the 73384 days in the supported range I hashed both directions, String,
ToYearString, ToMonthString, ToDayString, IsLeapMonth, Animal,
Festival and the result of ToGregorian. The digest is identical before and
after.
go test ./..., go test -race ./... pass, and statement coverage of
calendar/lunar stays at 100.0%.
I would also add a test that pins both tables to the definitions they replace:
one sub-test compares getDaysInYear against the bit counting formula for every
supported year, another rebuilds the running total step by step and compares it
against getOffsetInMonth, and a third checks that both tables cover the whole
years table. Existing tests do catch a plain off-by-one in the running total,
but they report it as eight unrelated looking date failures, while this one
fails on the first year and says which invariant broke.
What is left
The forward conversion is still O(years), it just does a lookup per step now
instead of recounting bits. Searching the running totals would make it O(1) as
well, but that changes the year-finding logic rather than only memoizing it, so
I left it out. Happy to look at it separately, and the same pattern applies to
calendar/hebrew, where getJDNInYear and getElapsedDays are 37% of a
1322 ns conversion.
I'd be happy to send a PR.
Thanks!
Hello,
Problem
Converting a date to lunar and back walks the year table instead of indexing it.
FromStdTimestarts atminYearand steps towards the wanted year, callinggetDaysInYearon every step (calendar/lunar/lunar.go:90):and
getDaysInYearrecounts the month bits of that year every time:ToGregorianhas the same shape throughgetOffsetInMonth, which sums thelength of every year since
minYear:So both conversions cost O(years since 1900), and the cost is visible in the
benchmarks:
A CPU profile of the conversion spends 26.6% of its time in
getDaysInYear.Suggested change
Everything those loops compute follows from the static
yearstable, so it canbe derived once: the number of days in each supported year, and the running
total of those days since
minYear.getDaysInYearthen becomes a lookup andgetOffsetInMonthbecomes O(1). The two tables are 201 ints each, built atpackage initialisation.
No logic changes, only the two accessors:
Correctness
Because both accessors are memoized pure functions of the same table, the
output cannot change, and I checked it rather than assuming: for every one of
the 73384 days in the supported range I hashed both directions,
String,ToYearString,ToMonthString,ToDayString,IsLeapMonth,Animal,Festivaland the result ofToGregorian. The digest is identical before andafter.
go test ./...,go test -race ./...pass, and statement coverage ofcalendar/lunarstays at 100.0%.I would also add a test that pins both tables to the definitions they replace:
one sub-test compares
getDaysInYearagainst the bit counting formula for everysupported year, another rebuilds the running total step by step and compares it
against
getOffsetInMonth, and a third checks that both tables cover the wholeyearstable. Existing tests do catch a plain off-by-one in the running total,but they report it as eight unrelated looking date failures, while this one
fails on the first year and says which invariant broke.
What is left
The forward conversion is still O(years), it just does a lookup per step now
instead of recounting bits. Searching the running totals would make it O(1) as
well, but that changes the year-finding logic rather than only memoizing it, so
I left it out. Happy to look at it separately, and the same pattern applies to
calendar/hebrew, wheregetJDNInYearandgetElapsedDaysare 37% of a1322 ns conversion.
I'd be happy to send a PR.
Thanks!