use ccalendar instead of np_datetime
State: closed

Do Not Merge

AFAICT the np_datetime.c versions of these functions are noticeably more performant than the cython versions.  I could use help verifying this observation since asv doesn't work well for me.

Best guess is that lookups in np_datetime.c's `days_per_month_table[2][12]` are more efficient than lookups in `ccalendar.days_per_month_array`, as there does not appear to be a way to instantiate the C data structure in cython (at least not as a module-global).


Labels:
name: Performance
description: Memory or execution speed performance
name: Clean

Comments:
what does the generated code look like? (for that function)
> what does the generated code look like? (for that function)

After stripping out a bunch of comments:

```
static CYTHON_INLINE __pyx_t_5numpy_int32_t __pyx_f_6pandas_5_libs_6tslibs_9ccalendar_get_days_in_month(int __pyx_v_year, Py_ssize_t __pyx_v_month, CYTHON_UNUSED int __pyx_skip_dispatch) {
  __pyx_t_5numpy_int32_t __pyx_r;

  __pyx_r = (__pyx_v_6pandas_5_libs_6tslibs_9ccalendar_days_per_month_array[(((12 * __pyx_f_6pandas_5_libs_6tslibs_9ccalendar_is_leapyear(__pyx_v_year)) + __pyx_v_month) - 1)]);
  goto __pyx_L0;

  /* function exit code */
  __pyx_L0:;
  return __pyx_r;
}```
I think this will all get inlined so performance shouldn't differ anyways, but FYI you can equivalently define/import `days_per_month_array` if you wanted to. 

```python
# ccalender.pyx
cdef int* days_per_month_array

#tslib.pyx
from ccalender cimport days_per_month_array
```
@chris-b1 I'm not sure we're on the same page as to the goal/problem.

- Seemingly-equivalent implementations in C appear to continue outperforming those in cython
- I think it has to do with the type of array that is used in the C file that can't be constructed as a global in the cython file. (see also comment above regarding cython's new "verbatim C" feature)
- I've been an advocate for reducing C dependencies and implementing things directly in cython; if there are unavoidable performance implications, I may need to rethink that position.
    - not having cython boilerplate leads (I think) to smaller .so files and (I expect) quicker compilation.  Not sure how much either of those matter.
- Profiling these functions is tricky, so all of this comes with a grain of salt

Thanks for the color, I was attempting to say that I think this part is false, or at least seemed to work with what I showed above
 > the type of array that is used in the C file that can't be constructed as a global in the cython file. (see also comment above regarding cython's new "verbatim C" feature)

Interesting (surprising) that performance is different, I will look closer
At least with `days_in_month` getting similar performance both ways
```python
# master
big_ts = pd.date_range('1999-01-01', periods=1_000_000, freq='H')
%timeit big_ts.daysinmonth
91.6 ms ± 11.7 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

# patch
big_ts = pd.date_range('1999-01-01', periods=1_000_000, freq='H')
%timeit big_ts.daysinmonth
94.7 ms ± 6.98 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)
```

patch
```diff
diff --git a/pandas/_libs/tslibs/fields.pyx b/pandas/_libs/tslibs/fields.pyx
index ccf67e765..be53dfe8b 100644
--- a/pandas/_libs/tslibs/fields.pyx
+++ b/pandas/_libs/tslibs/fields.pyx
@@ -14,13 +14,19 @@ from numpy cimport ndarray, int64_t, int32_t, int8_t
 cnp.import_array()

 from ccalendar import get_locale_names, MONTHS_FULL, DAYS_FULL
-from ccalendar cimport (get_days_in_month, is_leapyear, dayofweek,
+from ccalendar cimport (is_leapyear, dayofweek,
                         get_week_of_year, get_day_of_year)
 from np_datetime cimport (pandas_datetimestruct, pandas_timedeltastruct,
+                          days_per_month_table,
                           dt64_to_dtstruct, td64_to_tdstruct)
 from nattype cimport NPY_NAT


+@cython.wraparound(False)
+@cython.boundscheck(False)
+cdef inline int get_days_in_month(int year, int month) nogil:
+    return days_per_month_table[is_leapyear(year)][month - 1]
+
 def get_time_micros(ndarray[int64_t] dtindex):
     """
     Return the number of microseconds in the time component of a
```
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21549?src=pr&el=h1) Report
> Merging [#21549](https://codecov.io/gh/pandas-dev/pandas/pull/21549?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/f1aa08c4631aea435d66c161557e44f7c72a618e?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/21549/graphs/tree.svg?src=pr&token=eZ4WkYLtcO&height=150&width=650)](https://codecov.io/gh/pandas-dev/pandas/pull/21549?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #21549   +/-   ##
=======================================
  Coverage    91.9%    91.9%           
=======================================
  Files         153      153           
  Lines       49547    49547           
=======================================
  Hits        45537    45537           
  Misses       4010     4010
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.3% <ø> (ø)` | :arrow_up: |
| #single | `41.78% <ø> (ø)` | :arrow_up: |


------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21549?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21549?src=pr&el=footer). Last update [f1aa08c...f986a5c](https://codecov.io/gh/pandas-dev/pandas/pull/21549?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@jbrockmendel status of this? (as the DO NOT MERGE at the top is clear!)
If I understand @chris-b1 correctly then the perf discrepancy may not be an issue, in which case this is a clear (albeit small) improvement, so merge is OK.
thanks @jbrockmendel